Sanitise bin calculations #6212
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge branch 'main' into sanitise_bins # Conflicts: # man/ggplot2-ggproto.Rd
teunbrand
commented
Dec 4, 2024
| binwidth <- dual_param(binwidth, list(NULL, NULL)) | ||
| breaks <- dual_param(breaks, list(NULL, NULL)) | ||
| fun = "mean", fun.args = list(), | ||
| boundary = 0, closed = NULL, center = NULL) { | ||
| bins <- dual_param(bins, list(x = 30, y = 30)) |
There was a problem hiding this comment.
Ideally this is also done in setup_params(), but then the default argument to compute_group() should be bins = list(x = 30, y = 30), which causes a test to complain that there is a mismatch between compute_group() defaults and stat_*() constructor defaults.
teunbrand
commented
Dec 4, 2024
Comment on lines
-176
to
-180
| expect_snapshot_error(bin_breaks_width(3)) | ||
| expect_snapshot_error(comp_bin(dat, binwidth = letters)) | ||
| expect_snapshot_error(comp_bin(dat, binwidth = -4)) | ||
|
|
||
| expect_snapshot_error(bin_breaks_bins(3)) |
There was a problem hiding this comment.
No longer testing bin_breaks_width(3) and bin_breaks_bins(3) because these are always called from inside compute_bins(), which protects against improper ranges.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to fix #6207.
However, the linked issue is merely an occasion to sanitise bin calculations across ggplot2.
Most binning functions in ggplot2 acted similar, but not quite identical. An example of this is
stat_bin(boundary)has replaced thestat_bin(origin)argument since ggplot2 2.1.0, butstat_bin_2d(origin)continued to exist.The main changes in this PR:
compute_bins(), which is now used by all binning functionality. It is modelled after thestat_bin()functionality, as this was the most polished version.stat_bin()is able to deal with 0-width data elegantly, so this fixedgeom_bin_2d()should recognise 0-width data #6207.boundary/centerinstead oforigin. They were never formal arguments to most binning stats.stat_bin(keep.zeroes)introduced in Treatment options for zeroes in histograms #6139 was fulfilling the same/very similar role asstat_bin_2d(drop). However,stat_bin(drop)has been deprecated (also since 2.1.0). This PR renamesstat_bin(keep.zeroes)tostat_bin(drop), effectively resurrecting a formerly deprecated argument. There is no need for backward compatibility here sincekeep.zeroeswas introduced in this cycle of development.StatBin2dis now a subclass ofStatSummary2d, which prevents duplicated code.stat_bin(bins)can be a function now, purely for symmetry with thestat_bin(breaks, binwidth)arguments that can also be a function. You can use this to dobins = nclass.Sturgesfor example.Reprex from the linked issue:
Created on 2024-12-04 with reprex v2.1.1