Don't switch scale type when using defaults

[clauswilke]

Addresses this comment: #4450 (comment)

Reprex:

library(ggplot2)

v <- ggplot(faithfuld, aes(waiting, eruptions, fill = density)) +
  geom_tile()

options(ggplot2.continuous.fill = "viridis")
v

v + scale_fill_binned()

options(ggplot2.continuous.fill = scale_fill_viridis_c)
v

v + scale_fill_binned()

^{Created on 2021-04-28 by the reprex package (v1.0.0)}

[clauswilke]

FYI: The ubuntu/ R3.3 run failed at the setting-up-R stage. I'm not going to restart all CI runs just for that.

[thomasp85]

I think we should use this occasion to also add checks that the defaulted functions have the correct type and aesthetics, i.e. if the default is a function, expect the return value and see if it is a continuous scale for scale_colour_continuous() and if colour is part of the aesthetics

[clauswilke]

Just to be clear: Do you mean adding something like the following?

scale <- type(...)
if (!"colour" %in% scale$aesthetics | isTRUE(scale$is_discrete())) {
  stop("invalid scale function")
}
scale

[thomasp85]

Yes, exactly... I feel this would be a good catch

[clauswilke]

In fact, it would have helped me the other day when I was trying to figure out how to use this argument and was trying to provide a palette function. :-)

Will do today or tomorrow.

[clauswilke]

Ok, done I think.

[thomasp85]

Shouldn't we use this for the default discrete scale as well, in which case scale$is_discrete() should match some input argument?

[thomasp85]

Related issue for discrete scales: #4149

[clauswilke]

Ok, I'll look into that issue and see if I can fix it also.

[thomasp85]

Sorry for pilling onto what was effectively a documentation change🙂

[clauswilke]

@thomasp85 Do you want an error or a warning when the scale doesn't contain the right aesthetic? @yutannihilation suggested a warning here, I have currently implemented an error.

[clauswilke]

It looks like this is done differently for continuous and discrete scales. Continuous scales don't look at the defaults of other aesthetics. Maybe we should merge the current PR as is and defer a broader cleanup of this issue to the next major release, when we wanted to somewhat change how scales work anyways.

[yutannihilation]

I see. Sounds good to me.

[thomasp85]

We can merge this, but I'm very tempted to also fix the discrete scales for this release. The current behaviour is a mistake I think...

At least it should be avoided when the defaults are functions and only when a palette is provided

[thomasp85]

But to be frank that would probably be more confusing than simply not inheriting across aesthetics at all

[clauswilke]

Yeah, that's the point for me: I think this requires a little more careful thinking, and I'm not sure I want to rush this. I can make arguments for either way. It's certainly confusing to have different default color palettes for colour and fill, but then it's also confusing to have a scale_colour_*() applied to fill or to trigger a "incorrect scale" error if you change the default colour scale to a different function and then want to use the fill aesthetic.