-
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
Nonstandard aesthetics #2555
Nonstandard aesthetics #2555
Conversation
Two additional comments:
|
I do in ggraph but that shouldn’t weight against this PR |
@thomasp85 Also, the fix takes a minute. You simply need to use the provided Would you mind installing this PR and testing if it causes any problems with ggraph? |
sure, but not before next week. I’ll get back to you |
I really dislike |
Maybe have an aesthetic-less scale available where the aesthetic can be set in the function call, so you’d do: p + scale_brewer(..., aes = c(“fill”, “colour”)) It would require a huge amount of new exported functions, but api wise I think it is the most pretty approach |
I previously commented on the naming issue here: #2345 (comment) Words such as "color", "shape", "size" etc serve two purposes in the current ggplot2. On the one hand, they identify the aesthetic. On the other hand, they indicate the type of scale (color, shape, size, etc). If we only focus on the latter for a moment, it's clear that For these reasons, I'm very comfortable with |
Another way to think about it (which I'm proposing more as a thought experiment): Remove |
Ok, that seems like a compelling argument. We could also (eventually) consider making |
3841dda
to
1f09a31
Compare
I have rebased and added a few regression tests. |
Can you have a look at why travis is failing please? |
Not sure what the problem is. CRAN check works for me. I don't seem to be able to see the travis build log for this repository, and that makes figuring these things out more difficult. The one thing I noticed was an out-of-date .Rd file, so maybe that was it. I have committed it now. |
It looks like the Travis build timed out during installation of the dependencies. |
Either way it succeeded now. And I finally figured out where I can see the build log, so I'll be more informed in the future. |
can make full use of multiple aesthetics for one scale.
92a43a0
to
27788e1
Compare
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 think if we back off the user facing changes just slightly this is good to merge.
NEWS.md
Outdated
@@ -211,6 +215,10 @@ up correct aspect ratio, and draws a graticule. | |||
|
|||
* Legends no longer try and use set aesthetics that are not length one (#1932). | |||
|
|||
* All scales that are not position scales now have an `aesthetics` argument | |||
that can be used to set the aesthetics the scale works with, regardles of | |||
the name of the scale function. (@clauswilke) |
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.
For this version, maybe we could just add that argument to colour scales? You'll still be able to set it via ...
but it won't be so in your face.
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 problem is that you can't set the aesthetics via ...
, that's what brought me down this rabbit hole in the first place. The reason is that aesthetics
is hard-coded as an argument to discrete_scale()
or continuous_scale()
in all scale functions.
As an example, let's look at scale_colour_brewer()
, which is currently defined as follows:
scale_colour_brewer <- function(..., type = "seq", palette = 1, direction = 1) {
discrete_scale("colour", "brewer", brewer_pal(type, palette, direction), ...)
}
If we try to call it with a custom aesthetic, the following happens:
ggplot(iris, aes(Sepal.Length, fill = Species)) +
geom_density() +
scale_colour_brewer(aesthetics = "fill")
# Error in self$palette(n) : attempt to apply non-function
Since we provide an aesthetics
argument, it is handed off to discrete_scale()
via ...
, but now all the other arguments are shifted by one and so the palette function gets assigned to the wrong argument.
Here is a very simplified example to demonstrate the issue:
f <- function(a, b, c) {
print(a)
print(b)
print(c)
}
g <- function(...) {
f(4, 5, ...)
}
g(1)
# [1] 4
# [1] 5
# [1] 1
g(a = 1)
# [1] 1
# [1] 4
# [1] 5
I could process the ellipsis with match.call()
and then call the lower-level scale function with do.call()
, but that would be a lot of extra code in every single scale function, and it'd be not very readable. I can ponder some more whether there's some other way to make this happen, but I haven't come up with one yet.
Is there a way to test whether the ellipsis contains a specific argument, without actually evaluating the ellipsis? Maybe some rlang feature I'm not aware of?
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.
Ugh, of course, blech. What we just leave off the non-colour aesthetics for now and consider the user API in the future? I'd like to get this into this release, but I don't have the time to think through any big API changes.
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.
In that case, how do you feel about exporting manual_scale()
(possibly under a different name, e.g. scale_discrete_manual()
)?
Line 74 in 0dab044
manual_scale <- function(aesthetic, values, ...) { |
The problem is that there is currently literally no simple way of defining, for example, a discrete shape scale for an aesthetic other than shape
. One always has to reimplement manual_scale()
, and it's the reason I invented scale_discrete_manual()
in ggridges:
https://github.com/clauswilke/ggridges/blob/4e94e9a32b976c5a2eedc6fa04b104178f3e9767/R/scale-.R#L26
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, that sounds good, and I think that name is fine.
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, to summarize, I will:
- Revert addition of
aesthetics
argument for non-color scales. - Add and export
scale_discrete_manual()
which works just likemanual_scale()
.
I'll try to get this done by Friday.
I have made these changes now. The I also added three new generic scales that I think are needed, |
Thanks! |
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/ |
I realized yesterday that with very little work I could address most of the issues I raised in #2345. So I would like to propose this pull request to substantially improve ggplot2's handling of nonstandard aesthetics. The pull request consists primarily of removing hardcoded aesthetics names in various places, and a few minor adjustments to handle the fallout. All the regression tests pass, and none of the changes should impact anybody who is using ggplot2 as they always have. Below follow some examples of what this code can do.
First, we can now specify the aesthetics independently of the scale name,
e.g., define the fill scale using
scale_color_brewer()
:Second, we can specify multiple aesthetics at once. The legends get merged if appropriate:
This also works if the aesthetics represent different data columns:
The limits get merged automatically, as can be seen here more clearly:
All of this also works with non-standard aesthetics, which I have implemented in ggridges.
First an example with
guide_legend()
:And now an example with
guide_colorbar()
: