-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Show proper error when both x and y are specified on stat_density() and stat_ecdf() #4914
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
Show proper error when both x and y are specified on stat_density() and stat_ecdf() #4914
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.
We are now doing unit tests for all errors so can I get you to add tests for this
|
Sure |
|
It seems this existing code will be broken by this change. I'm still thinking, but this might mean the same case can be found with library(ggplot2)
do_plot <- function() {
ggplot(mtcars, aes(mpg, factor(cyl))) +
stat_density(aes(fill = after_stat(density)), geom = "raster", position = "identity")
}
do_plot()devtools::load_all("~/GitHub/ggplot2/")
#> ℹ Loading ggplot2
do_plot()
#> Error in `stat_density(aes(fill = after_stat(density)), geom = "raster", position =
#> "identity")`:
#> ! Problem while computing stat.
#> ℹ Error occurred in the 1st layer.
#> Caused by error in `setup_params()` at ggplot2/R/ggproto.r:182:16:
#> ! `stat_density()` must only have an x or y aesthetic.Created on 2022-07-27 by the reprex package (v2.0.1) |
|
@yutannihilation The example you found that breaks is an important use case I think. Fundamentally this highlights the core problem, which is that we don't explicitly distinguish between aesthetics given to the stat and to the geom. We may have to accept the confusing error message. |
|
Thanks. Yes, I'm almost giving up this pull request. Rather, my current concern is if ggplot(mtcars, aes(mpg, factor(cyl))) +
stat_bin(aes(fill = after_stat(count)), geom = "raster", position = "identity") |
|
As this pull request seems impossible, I'm giving up this. Sorry that I didn't come up with the usage. |


Fix #4776