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

Make geom_col() ignore data$width #3194

Conversation

yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Mar 19, 2019

Fix #3142

Currently, geom_bar() does ignore width mapping, but geom_col() does not. Here's my thinking:

  • We want to enforce a constant bar width within a panel, so width cannot be an aesthetic.
  • Yet, the widths can vary among panels, so we need to pass widths per bar via data, not a single value via param.
  • data$width should be used only when it's a calculated value by Stat. But, as geom_col() is not the case, it should ignore data$width.

Note that, this doesn't prevent variable widths when geom_bar() uses such Stat as

  • StatIdentity
  • some Stat that actually returns variable widths

but I think geom_col() should ignore width for consistency at least.

Current HEAD

library(ggplot2)
library(patchwork)

d <- data.frame(x = c(1,2,3,3,3))

p1 <- ggplot(d) + geom_bar(aes(x, width = x / 2), alpha = 0.7) + ggtitle("bar")
#> Warning: Ignoring unknown aesthetics: width
p2 <- ggplot(d) + geom_col(aes(x, x, width = x / 2), alpha = 0.7) + ggtitle("col")
#> Warning: Ignoring unknown aesthetics: width

p1 / p2
#> Warning: position_stack requires non-overlapping x intervals

Created on 2019-03-19 by the reprex package (v0.2.1.9000)

This PR

library(ggplot2)
library(patchwork)

d <- data.frame(x = c(1,2,3,3,3))

p1 <- ggplot(d) + geom_bar(aes(x, width = x / 2), alpha = 0.7) + ggtitle("bar")
#> Warning: Ignoring unknown aesthetics: width
p2 <- ggplot(d) + geom_col(aes(x, x, width = x / 2), alpha = 0.7) + ggtitle("col")
#> Warning: Ignoring unknown aesthetics: width

p1 / p2

Created on 2019-03-19 by the reprex package (v0.2.1.9000)

Copy link
Member

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

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

Do you want to mention in the documentation that geom_col() enforces equal column widths?

# To ensure bar widths are constant within a PANEL, `width` is not allowed
# to be mapped to data. So, even if the `data` already contains `width`, it
# should be ignored except when the width is calculated by Stat. But, as
# geom_col() uses stat_identity() fixedly, this is not the case.
Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to point out that if somebody writes code such as stat_bin(geom = "col") they could arrive here with a calculated width column, but that would probably be considered inappropriate use then. May be worth a mention in the comment, for the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@yutannihilation
Copy link
Member Author

Do you want to mention in the documentation that geom_col() enforces equal column widths?

Yes, I think it's good to have it documented clearly.

@yutannihilation
Copy link
Member Author

Hmmm, sorry, after thinking about stat_bin(geom = "col"), now I feel this PR just creates another inconsistency. Let me withdraw this. Really sorry for taking your time, Claus.

To fix this, I think we need to remove data$width from geom_bar() too, but we don't have any measure to propagate calculated widths from a Stat to a Geom other than data (for now).

library(ggplot2)

ggplot(diamonds) +
  stat_bin(aes(carat, width = stat(count)), geom = "col") 
#> Warning: Ignoring unknown aesthetics: width
#> `stat_bin()` using `bins = 30`. Pick better value with `binwidth`.

ggplot(diamonds) +
  stat_bin(aes(carat, width = stat(count)), geom = "bar") 
#> Warning: Ignoring unknown aesthetics: width
#> `stat_bin()` using `bins = 30`. Pick better value with `binwidth`.
#> Warning: position_stack requires non-overlapping x intervals

Created on 2019-03-22 by the reprex package (v0.2.1)

@lock
Copy link

lock bot commented Feb 28, 2020

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 Feb 28, 2020
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.

geom_bar()/geom_col() erroneously warn that they ignore width aesthetic
2 participants