-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Finer control over aesthetic evaluation #3534
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.
Seems fine to me, except that mod()
is too short; it's too likely to conflict with other packages and it's not going to be used that commonly so it doesn't need to be really short.
any good suggestions on name? |
I like library(ggplot2)
ggplot(mpg, aes(class, hwy)) +
geom_boxplot(aes(colour = ifelse(class %in% c("pickup", "suv", "minivan"), "large car", "small car"))) +
scale_color_discrete(name = NULL) Created on 2019-09-19 by the reprex package (v0.3.0) |
Cool, this solves the frequent request like #3485. Minor question, do we need to be tolerant for ggplot(mpg, aes(class, hwy)) +
geom_boxplot(aes(color = class, fill = mapped(alpha(color, alpha))), alpha = 0.4)
#> Error in as.character(col) %in% "0": object 'color' not found |
Yeah — I thought about that as well. I think we should |
That's an excellent point. Aesthetics names get standardized as described/implemented here: Lines 150 to 163 in b560662
Ideally the same rules should apply inside |
I have implemented a way to recurse through aesthetic expressions ago substitute names to standard aesthetic nomenclature. We should consider this carefully though as I don't think many people are aware of the old-style support and may be surprised that their variables are getting renamed..? |
So... this PR has experienced a little feature creep 🙂 I've added a new control function as well called API (continuing with the same example): ggplot(mpg, aes(x = stage(stat = class, geom = x + 0.5), y = hwy)) +
geom_boxplot(aes(colour = class, fill = mapped(alpha(color, alpha))), alpha = 0.4) |
@lionel- can I get you to have a look at the meta-programming stuff and make sure I've not done something terribly stupid? |
For the name, as this functionality is similar to |
R/aes-calculated.r
Outdated
@@ -36,7 +73,12 @@ is_dotted_var <- function(x) { | |||
is_calculated_aes <- function(aesthetics) { | |||
vapply(aesthetics, is_calculated, logical(1), USE.NAMES = FALSE) | |||
} | |||
|
|||
is_mapped_aes <- function(aesthetics) { |
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.
These semantics feel a bit sloppy to me because they allow users to affect the interpretation of an expression by using mapped()
or stage()
deep in the call, even though that applies to the whole expression, not subparts. It seems it would be cleaner to require the qualifier to be at top-level.
Then you could check for a mapped()
call at top level, and bind mapped
to a function that fails with an error message in the function-level of the data mask.
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.
On a side note, I find it confusing to have is_
predicate return length != 1 vectors. I think an is_
function should always be safe to use in if ()
(length 1, no missing value). In rlang I use the are_
prefix for vectorised predicates (but this convention wasn't picked up anywhere else I think).
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.
The whole is_mapped
and is_mapped_aes
setup is borrowed completely from the way stat()
calls are handled. @hadley do you think there is reason to rewrite them both?
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.
Yeah, we should definitely only allow these at the top-level for new code. It's probably too late to change the behaviour of stat()
, unless you want to deprecate it first.
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.
ok, so I'll provide simplified parsers for mapped()
(or whatever it ends up being called) and stage()
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.
Also expected:
is_call(~ foo())
#> [1] TRUE
is_call(~ foo(), "foo")
#> [1] FALSE
is_call(~ foo(), "~")
#> [1] TRUE
A quosure is not a call, and a formula is a call to ~
. Some functions in rlang will unwrap formulas and quosures automatically, but is_call()
is too low level for that.
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.
Ah... I misunderstood... I thought that FALSE
was an error and that it would be fixed with next release...
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.
so... would this be the correct way to test for whether a quosure is of a specific call?
is_call(quo_squash(t), 'test')
or are there any gotchas to that approach?
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.
You should almost never use quo_squash()
, its name is meant to be scary ;). quo_get_expr()
is the way to extract an expression.
-
If you know you have a quosure, use
quo_is_call()
oris_call(quo_get_expr())
. -
If you'd like to automatically unwrap quosures and formulas (this can be tricky and cause unexpected results), use
is_call(get_expr())
.
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.
get_expr(quote(foo()))
#> foo()
get_expr(~ foo())
#> foo()
get_expr(quo(foo()))
#> foo()
On old-style support: I think On the name of the function |
|
Ah, Ok. |
As for old-style name. I do t suggest we deprecate them. But we need to be careful about this as there is a difference in supporting them as argument names and substituting their use inside expressions. The later will mask any variable that coincide with an old-style name |
R/aes-calculated.r
Outdated
@@ -36,7 +73,12 @@ is_dotted_var <- function(x) { | |||
is_calculated_aes <- function(aesthetics) { | |||
vapply(aesthetics, is_calculated, logical(1), USE.NAMES = FALSE) | |||
} | |||
|
|||
is_mapped_aes <- function(aesthetics) { |
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.
Yeah, we should definitely only allow these at the top-level for new code. It's probably too late to change the behaviour of stat()
, unless you want to deprecate it first.
…g docs for aes evaluation
Can you write a NEWS bullet please? I find them very helpful for giving me the PR context (which I've usually forgotten, and I'd prefer not to have to re-read the entire thread) |
news bullet added @hadley |
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.
Overall interface looks good to me. I didn't closely review the implementation.
can you approve if you trust me on the implementation 🙂 |
This PR implements a parallel to
stat()
, calledmod()
that allows you to mark aesthetics for evaluation just before they are send to the draw method in the geom. At this point the data has been mapped so expressions insidemod()
will work with mapped data.The prime use case for this is for tying colour and fill values together:
As can be seen the legend needs some work, but I open now so we can discuss this feature