-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Allow functional limits in continuous scales #2334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow functional limits in continuous scales #2334
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a small change and it seems useful, so lets do it! A few suggestions in the comments.
Also needs one or two unit tests. Can you please have a look at the existing tests and think about where they should go?
R/scale-.r
Outdated
# if scale contains a NULL, use the default scale range | ||
# if scale contains a NA, use the default range for that axis, otherwise | ||
# use the user defined limit for that axis | ||
get_limits = function(self) { | ||
if (self$is_empty()) return(c(0, 1)) | ||
|
||
if (!is.null(self$limits)) { | ||
if (is.function(self$limits)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're adding another condition here, can you please reframe the if statement along these lines:
if (is.null()) {
} else if (is.function()) {
} else if (is.numeric()) {
} else {
stop("Informative error message", call. = FALSE)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did rework this conditional to be more intuitive but since get_limits()
is called by both discrete and continuous scales and can be passed character, numeric, POSIXt and now function classes, I ultimately did not add an explicit is.numeric
conditional nor an error message. Happy to update further if necessary.
Hey @econandrew if you're still interested in this PR, I'm around (:wave: summer intern!) and can help you get this over the finish line if you'd like. Let me know if I can help in anyway. |
@dpseidel would you consider finishing this off? |
Yep happy to. |
Merge remote-tracking branch 'upstream/master' into allow-functional-limits # Conflicts: # R/scale-.r
The diff seems to include spurious changes which makes it hard to review. |
Sorry, yes, I got a little nit picky on the NEWS doc, clearly that should be in a different PR. All cleaned up now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for finishing it of Dana
This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/ |
An implementation of the idea in #2307, for consideration.
Motivation is to allow programmatic, data-agnostic control of scale limits, taking advantage of rather than having to replicate ggplot's aesthetic calculations. For example, suppose you want limits to always be a multiple of 50: