Color scale reuse

[steveharoz]

This PR takes a different approach to #5471 based on a suggestion by @teunbrand.

Changes

1. A general purpose factory function called reuse_scale can copy a colour scale for use with another color-based aesthetic. For example scale_fill_grey <- reuse_scale(scale_colour_grey, "fill")
2. All scale_fill* functions were moved to scale-fill.R.
3. The remaining functions from zxx.R were moved to scale-colour-ordinal.R and scale-color.R. zxx.R is no longer needed.

Main benefit

1. Argument defaults and internal logic for colour and fill scales are only specified once, so code duplication is substantially reduced.

Bonus benefit

1. This change moves in the direction of using fill as a test case for extensibility of color scales. A goal is that people making extensions to ggplot who want to add a new color aesthetic can simply use the scale-fill.R file as a template. This update accomplishes most of that. The only aspect remaining is that guide_colourbar still has fill hardcoded.
2. Making any improvements to ggplot's color scales only has to be done once. I found a number of places where color and fill functions were doing the same thing in different ways.

To do

Before finalizing PR:

• Fix build errors related to how type is handled for US vs UK spelling
• See why vignettes aren't building (probably same bug, but double check)
• Improve documentation for reuse function (how do I add an example that I don't want to run?)

Maybe address in another PR:

• Now that files like scale-hue.R only have colour functions in them (no fill functions), rename to scale-colour-hue.R.
• Remove fill from the hardcoded acceptable aesthetics in guide_colourbar. Then modify guide_colourbar to allow fill to be added after the fact. Add fill to guide_colourbar in scale-fill.R.

My tests

ggplot(mtcars) + aes(wt, mpg, color="blah") + geom_point(shape=25, size=3, stroke=2)
ggplot(mtcars) + aes(wt, mpg, color=factor(cyl)) + geom_point(shape=25, size=3, stroke=2)
ggplot(mtcars) + aes(wt, mpg, color=hp) + geom_point(shape=25, size=3, stroke=2)

ggplot(mtcars) + aes(wt, mpg, fill="blah") + geom_point(shape=25, size=3, stroke=2)
ggplot(mtcars) + aes(wt, mpg, fill=factor(cyl)) + geom_point(shape=25, size=3, stroke=2)
ggplot(mtcars) + aes(wt, mpg, fill=hp) + geom_point(shape=25, size=3, stroke=2)

ggplot(mtcars) + aes(wt, mpg, color=factor(cyl)) + geom_point(shape=25, size=3, stroke=2) + scale_color_discrete()
ggplot(mtcars) + aes(wt, mpg, color=hp) + geom_point(shape=25, size=3, stroke=2) + scale_color_binned()
ggplot(mtcars) + aes(wt, mpg, color=hp) + geom_point(shape=25, size=3, stroke=2) + scale_color_continuous()

ggplot(mtcars) + aes(wt, mpg, fill=factor(cyl)) + geom_point(shape=25, size=3, stroke=2) + scale_fill_discrete()
ggplot(mtcars) + aes(wt, mpg, fill=hp) + geom_point(shape=25, size=3, stroke=2) + scale_fill_binned()
ggplot(mtcars) + aes(wt, mpg, fill=hp) + geom_point(shape=25, size=3, stroke=2) + scale_fill_continuous()
 # fail
ggplot(mtcars) + aes(wt, mpg, color=factor(cyl), fill=factor(cyl)) + geom_point(shape=25, size=3, stroke=2) + scale_color_discrete(aesthetics = c("color"))
ggplot(mtcars) + aes(wt, mpg, color=factor(cyl), fill=factor(cyl)) + geom_point(shape=25, size=3, stroke=2) + scale_color_discrete(aesthetics = c("fill"))
 # fail
ggplot(mtcars) + aes(wt, mpg, color=factor(cyl), fill=factor(cyl)) + geom_point(shape=25, size=3, stroke=2) + scale_color_discrete(aesthetics = c("color", "fill"))
 # fail
ggplot(mtcars) + aes(wt, mpg, color=factor(cyl), fill=factor(cyl)) + geom_point(shape=25, size=3, stroke=2) + scale_color_discrete(aesthetics = c("colour", "fill"))
ggplot(mtcars) + aes(wt, mpg, color=factor(cyl), fill=factor(cyl)) + geom_point(shape=25, size=3, stroke=2) + scale_fill_discrete()
ggplot(mtcars) + aes(wt, mpg, color=factor(cyl), fill=gear) + geom_point(shape=25, size=3, stroke=2) + scale_fill_gradient()
ggplot(mtcars) + aes(wt, mpg, color=hp, fill=hp) + geom_point(shape=25, size=3, stroke=2) + scale_fill_binned()


ggplot(faithfuld, aes(waiting, eruptions, fill = density)) + geom_tile() + scale_fill_binned()
ggplot(faithfuld, aes(waiting, eruptions, fill = density, color = density)) + geom_tile(linewidth = 1) + 
  scale_fill_viridis_b() + scale_color_viridis_b(option = "C")

[teunbrand]

Hi Steve, thanks for the PR! I haven't looked in-depth yet, but here are some early thoughts.
I really like seeing that so many duplicated lines of code can be reduced, that's great!
I'm not entirely sure about reuse_all_colour_scales() though, because of the following reasons.

• It invites users to ask for similar functions for other aesthetics that are harder to implement.
• The pattern of defining scales as NULL and only filling them in later is harder to reason about than necessary.
• I'm uncertain that binding the produced scales to the parent environment is fool-proof enough. If a user just calls it in their interactive session, are you then just populating their global environment with new scales?

If we could find a way to pass additional arguments to the reused scales, we might also simplify all (or many of) the scale_{aes}_manual() and scale_{aes}_identity() functions. Lastly, I know that I'm the one who suggested the reuse_scale() name, but perhaps there are better names out there that more clearly convey the intent of the function (I'm willing to accept there might not be).

[teunbrand]

I understand something needs to be declared for the documentation to work out, but I don't think it helps code readability to assign NULL here. It might be clearer to just use reuse_scale() directly here (and other places where scales are declared as NULL), because then one doesn't have to lookup where and how these scales are instantiated.

[steveharoz]

Yeah, I've waffled back and forth on how to initialize the fill scales.

On one hand, as you said, readability is best when it's defined in place. However, if each is defined individually, they'd be defined twice with the reuse_all function. Also, defining each one in place makes it easy to miss one or accidentally assign the wrong colour scale to a fill scale.

How about using comments like this?

scale_fill_brewer <- NULL  # definition added in `reuse_all_colour_scales()`

[steveharoz]

Also, an option which may make more sense now that fill scales are much simpler is moving all fill scales into a scale-fill.r file. If there's a scale-colour.r file, might as well be one for fill.

[thomasp85]

I agree with Teun that having this delayed assignment is not a good direction for readability. Also, the point of the automatic creation kind of falls apart when you have to keep track of all the assignments anyway. So, the optimal approach IMO is to forego the whole reuse_all pattern

[steveharoz]

@thomasp85
Sounds good. That makes sense. Any thoughts on moving all the fill scales to a scale-fill.r file? I think it'd help people making extensions if we can say "To make a new color scale, copy everything in the scale-fill.r but replace 'fill' with your own."

See also #5484 (comment)

[thomasp85]

Yeah, that could work

[steveharoz]

An overall goal here is to try to make the fill aesthetic as a built-in test of ggplot's extensibility. If ggplot implements the fill aesthetic just like an extension package would define it's own custom color aesthetic, then any issues or pain points regarding color aesthetics are clear within the ggplot build.

Another step in that overarching goal is to remove "fill" from available_aes in GuideColourbar and only add it after the fact (like an extension would). But that's a whole beast on its own.