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

Setting options(ggplot2.discrete.fill) overrides for aes(colour) with bad colourmap #4149

Closed
apoorvalal opened this issue Jul 28, 2020 · 5 comments · Fixed by #4470
Closed
Labels
bug an unexpected problem or unintended behavior scales 🐍
Milestone

Comments

@apoorvalal
Copy link

apoorvalal commented Jul 28, 2020

I know the option to set colour palette is in its early stages, so wanted to flag an error I noticed.

Looks like setting ggplot2.discrete.fill overwrites the colourmap for colour (which should be unchanged based on the documentation?) with a cmap that is both ugly and unintelligible (and it hides the legend too for some reason).

``` r
library(ggplot2)
(p1 = ggplot(mpg, aes(class)) + geom_bar(aes(fill = drv)))

# unrelated plot 
(p2 = ggplot(mtcars, aes(x = wt,  y = mpg)) +
  geom_point(aes(colour = as.factor(cyl))))

# this should only apply to p1
options(ggplot2.discrete.fill    = function() scale_fill_viridis_d())
# as it does
p1

# but it also mysteriously overrides p2's colourmap 
p2

I'm running ggplot 3.3.2 in R 3.6.3 on a machine running ubuntu 18.04 LTS.

@yutannihilation
Copy link
Member

yutannihilation commented Jul 28, 2020

Thanks, confirmed. Overriding is for convenience and it works perfectly when a vector of colors is supplied (or specifying aesthetics = c("colour", "fill") in the scale function. But, it also leads us to unexpected behaviours like the one you encountered. here's another bad example. I agree this needs to be improved somehow.

library(ggplot2)

p <- ggplot(mpg, aes(class)) + geom_bar(aes(fill = drv))

# Correct, and works as expected
withr::with_options(
  list(ggplot2.discrete.fill = function() scale_fill_viridis_d()),
  print(p)
)

# Incorrect, and fails with a cryptic error
withr::with_options(
  list(ggplot2.discrete.fill = function() scale_colour_viridis_d()),
  print(p)
)
#> Error: Unknown colour name: f

@thomasp85
Copy link
Member

@yutannihilation do you have any thoughts on how to best approach this?

@yutannihilation
Copy link
Member

yutannihilation commented Apr 27, 2021

I was thinking if it's possible to check aesthetics of the scale and tweak it (with some warning). For example:

scale_colour_discrete <- function(..., type = getOption("ggplot2.discrete.colour", getOption("ggplot2.discrete.fill"))) {
  # TODO: eventually `type` should default to a set of colour-blind safe color codes (e.g. Okabe-Ito)
  type <- type %||% scale_colour_hue
  if (is.function(type)) {
    scale <- type(...)
    if (!"colour" %in% scale$aesthetics) {
      warn("...some nice warning...")
      scale$aesthetics <- c(scale$aesthetics, "colour")
    }
    scale
  } else {
    scale_colour_qualitative(..., type = type)
  }
}

@thomasp85
Copy link
Member

@cpsievert are you relying on the colour->fill (or reverse) fallback in your theming packages? I'm very tempted to remove it to get it in line with how defaults are handled for continuous scales

@cpsievert
Copy link
Contributor

@thomasp85 pretty sure I set both of them directly, so feel free to separate them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior scales 🐍
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants