Only call zero_range() and as.numeric() in transformed space#5304
Merged
teunbrand merged 4 commits intotidyverse:mainfrom Sep 12, 2023
Merged
Only call zero_range() and as.numeric() in transformed space#5304teunbrand merged 4 commits intotidyverse:mainfrom
zero_range() and as.numeric() in transformed space#5304teunbrand merged 4 commits intotidyverse:mainfrom
Conversation
In case the `limits` in data space don't support conversion to numeric
thomasp85
approved these changes
May 10, 2023
Member
thomasp85
left a comment
There was a problem hiding this comment.
LGTM - @clauswilke can you see any issues with this change?
teunbrand
approved these changes
Sep 12, 2023
Collaborator
teunbrand
left a comment
There was a problem hiding this comment.
This looks good to me. Would you mind harmonising the news bullet with the latest changes @DavisVaughan or would you prefer me to do that?
Member
Author
|
@teunbrand if you could please take over I would really appreciate it! It would probably be much faster for you at this point. |
Collaborator
|
Thanks @DavisVaughan for the PR! |
This file contains hidden or 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
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.
In case the
limitsin data space don't support conversion to numericI've been working on scale support for {clock} types in r-lib/clock#345. These include scales for things like
year_month_day<month>, a "year-month" style date vector, andyear_week_day<week>, a "year-week" style date vector.I've got pretty much everything working, but there is a show stopping issue with
ScaleContinuous$get_breaks()that I've hopefully fixed here.It looks like
scales::zero_range()is being called onas.numeric(limits), wherelimitsis in the original data space. This is problematic for clock, because calendar types likeyear_month_dayandyear_week_daydo not supportas.numeric(), so the limits can't be converted directly. However, they do supportscale$trans$transform()to convert from a clock type in data space to a numeric value in transformed space.IIUC, I think that the intention of
scales::zero_range()is that it should only be called on objects in transformed space anyways. Looking at other uses of it in ggplot2:ScaleContinuous$get_minor_breaks()it is called onlimitswhen they are in transformed space, so this PR basically just makesget_breaks()consistent with that.ggplot2/R/scale-.R
Line 692 in aa2ddbe
get_breaks()just above where this change happens it is called ondomainwhich is in transformed spaceggplot2/R/scale-.R
Line 651 in aa2ddbe
bin_breaks_bins()it is called onx_rangewhich comes through as a$dimension()call, and the dimension is always returned in transformed spaceggplot2/R/bin.R
Line 112 in aa2ddbe
Hopefully this is enough evidence to support this change.
I couldn't think of a good test for this, so if anyone has a better idea, feel free to take over.