Skip to content
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

Warn when using constant aesthetics #5256

Merged
merged 4 commits into from
Apr 15, 2023

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Apr 1, 2023

This PR aims to fix #5253.

Briefly, it adds a warning when using an all-constant mapping in a layer:

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

p <- ggplot(mtcars, aes(x = 1)) +
  geom_point(aes(y = 2))
b <- ggplot_build(p)
#> Warning: All aesthetics have length 1, but the data has 32 rows in `geom_point()`.
#> ℹ Did you mean to use `annotate()`?

It does take into account inherited aesthetics, so no warning is issued here:

p <- ggplot(mtcars, aes(x = seq_along(mpg))) +
  geom_point(aes(y = 2))
b <- ggplot_build(p)

There is an ambiguous case where I was unsure a warning should be emitted. If a layer inherits an aesthetic that it does not use, should the warning been thrown for the aesthetics it does use?

# To warn or not to warn?
p <- ggplot(mtcars, aes(label = cyl)) +
  geom_point(aes(x = 1, y = 1))
b <- ggplot_build(p)

Created on 2023-04-01 with reprex v2.0.2

The geom_abline()/geom_vline()/geom_hline() are excluded from this check. Extenders can exclude their geoms from this check by using the same exclusion flag (check_constant_aes = FALSE in the geom's ggproto fields).

R/layer.R Outdated Show resolved Hide resolved
Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd be happy if @clauswilke had a look as well

@clauswilke
Copy link
Member

Yes, I think it's fine. Worst case scenario there will be some false positives and we can deal with them then.

@teunbrand teunbrand merged commit e3b7822 into tidyverse:main Apr 15, 2023
@teunbrand teunbrand deleted the warn_constant_aes branch April 15, 2023 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surprising behaviour when aes() is used to map all aesthetics to constant values
3 participants