Skip to content

Fix issue 1546, fill aesthetic does not show in the legend guide for stat_summary #2567

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

Merged
merged 3 commits into from
May 10, 2018

Conversation

clauswilke
Copy link
Member

I think the way to fix this issue is to give geom_smooth() a first-class se parameter that is considered when drawing the panel. This allows the geom and the legend drawing code to always be in synch. See resulting behavior in the examples below.

One note: Currently one regression test fails because the geom_smooth() and draw_group() functions have different default arguments and the test checks for that. I think the test is wrong in this particular case, but I wanted to bring this up for discussion. Relevant lines in the code, with explanation for why it should be this way:
https://github.com/clauswilke/ggplot2/blob/ed016144c97cf67424498453a2f4e06925fdba92/R/geom-smooth.r#L119-L126

Examples of geom_smooth() with stat_summary():

library(ggplot2)
ggplot(mtcars, aes(cyl, mpg, fill = factor(am))) +
  geom_smooth(stat = "summary")
#> No summary function supplied, defaulting to `mean_se()

ggplot(mtcars, aes(cyl, mpg, fill = factor(am))) +
  geom_smooth(stat = "summary", se = FALSE)
#> No summary function supplied, defaulting to `mean_se()

# stat_summary doesn't have an `se` parameter, hence
# default is `se = FALSE`
ggplot(mtcars, aes(cyl, mpg, fill = factor(am))) +
  stat_summary(geom = "smooth")
#> No summary function supplied, defaulting to `mean_se()

ggplot(mtcars, aes(cyl, mpg, fill = factor(am))) +
  stat_summary(geom = "smooth", se = TRUE)
#> No summary function supplied, defaulting to `mean_se()

And, for completeness, all combinations of geom_smooth() / stat_smooth() with se = TRUE / se = FALSE work as expected:

library(ggplot2)
ggplot(iris, aes(Sepal.Length, Sepal.Width, color = Species, fill = Species)) +
  geom_smooth()
#> `geom_smooth()` using method = 'loess' and formula 'y ~ x'

ggplot(iris, aes(Sepal.Length, Sepal.Width, color = Species, fill = Species)) +
  geom_smooth(se = FALSE)
#> `geom_smooth()` using method = 'loess' and formula 'y ~ x'

ggplot(iris, aes(Sepal.Length, Sepal.Width, color = Species, fill = Species)) +
  stat_smooth()
#> `geom_smooth()` using method = 'loess' and formula 'y ~ x'

ggplot(iris, aes(Sepal.Length, Sepal.Width, color = Species, fill = Species)) +
  stat_smooth(se = FALSE)
#> `geom_smooth()` using method = 'loess' and formula 'y ~ x'

Created on 2018-05-10 by the reprex package (v0.2.0).

@hadley
Copy link
Member

hadley commented May 10, 2018

I think this approach is reasonable; the way parameters works is all a little hacky, so you should feel free to disable the test for this case (IIRC there are already a few other geoms where it's disabled)

@clauswilke
Copy link
Member Author

I've disabled the consistency check for geom_smooth(). (You were correct, there was already a list of such geoms.) I have added two simple visual tests, since there weren't any for geom_smooth() or stat_smooth() yet.

@hadley hadley merged commit 8e365a4 into tidyverse:master May 10, 2018
@hadley
Copy link
Member

hadley commented May 10, 2018

Thanks!

@lock
Copy link

lock bot commented Nov 17, 2018

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/

@lock lock bot locked and limited conversation to collaborators Nov 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants