Proposal: factory pattern for color scales

[steveharoz]

Currently, each color scale (hue, grey, viridis, etc.) needs a duplicate function for each aesthetic it maps to. Here are the functions in scale-grey.r:

scale_colour_grey <- function(..., start = 0.2, end = 0.8, na.value = "red", aesthetics = "colour") {
  discrete_scale(aesthetics, palette = grey_pal(start, end),
    na.value = na.value, ...)
}

scale_fill_grey <- function(..., start = 0.2, end = 0.8, na.value = "red", aesthetics = "fill") {
  discrete_scale(aesthetics, palette = grey_pal(start, end),
    na.value = na.value, ...)
}

Problems with that approach:

1. The argument defaults and internal logic are duplicated, so changing one but forgetting to change the other could cause confusing inconsistent behavior.
2. Adding a new color aesthetic like linecolor or outliercolor requires duplicating the function for each color scale. As a result, adding a new color-based aesthetic is pretty cumbersome.

Why the aesthetics argument isn't a sufficient solution:

A new color aesthetic still needs defaults that behave like the others. If you map a new aesthetic in the same way that you map fill, but fill uses default the color scale, they won't match. So you'll need to duplicate the scale functions for hue, gradient, and viridis just for defaults to match the behavior of color and fill.

Proposed approach: Create factory methods

Instead of scale_colour_grey() and scale_fill_grey defined above, create one function takes an aesthetic as a parameter and returns a scale function:

scale_grey_factory <- function(aesthetic) {
  function(..., start = 0.2, end = 0.8, na.value = "red", aesthetics = aesthetic) {
    discrete_scale(aesthetics, palette = grey_pal(start, end),
    na.value = na.value, ...)
  }
}

scale_colour_grey <- scale_grey_factory("colour")
scale_fill_grey <- scale_grey_factory("fill")

This approach avoids duplicating the argument defaults. Also, adding another color aesthetic only requires one line of code per color scale:

scale_linecolour_grey <- scale_grey_factory("linecolour")
scale_outliercolor_grey <- scale_grey_factory("outliercolor")

TODOs

• Check if I missed any scales?
• Test more thoroughly to ensure everything works and looks unchanged for users.
• Roxygen works correctly, but I had to put the factory functions at the top.

[thomasp85]

I'm fine with this idea in general, but we are not going to add additional speciality scales for various colours so this PR will only be about removing code duplication in the current code base

[steveharoz]

@thomasp85 That's fine. Would you be ok with exporting the factory functions, so people making extensions can add specialty color scales? As it stands, the logic for default color scales (what to do when a scale isn't specified) is locked away in ggplot.

Also, a question: Can one of you explain or point me to info about how the type argument is used? I don't see any example functions that pass in a value for that argument. For example, why would someone call scale_colour_continuous(type="viridis") instead of just calling scale_color_viridis_c()? Is it just for setting different continuous/discrete defaults via options()?

[teunbrand]

I think the whole scale_{aes}_{palette}() family of functions was a design mistake that should just be continuous/discrete/binned versions with easier options to set palettes e.g. palette = "viridis". However, we'll probably never get out of the current situation without breaking a lot of existing code, so we might as well make the best of it.

Is it just for setting different continuous/discrete defaults via options()?

That is how I understand that argument. It has been proposed to move away from this in favour of setting default scales in the theme() though (see #2691).

[steveharoz]

@teunbrand Thanks. Yeah, I partly agree about using parameters, but I also see the value in hard-coded functions for autocomplete.

[teunbrand]

I was thinking about this PR, and could we not generalise the idea away from multiple factories to a single function to copy a scale for another aesthetic?

Quick sketch blind to any specifics:

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

reuse_scale <- function(scale, aesthetic, scale_arg = caller_arg(scale)) {
  check_function(scale)
  if (!"aesthetics" %in% fn_fmls_names(scale)) {
    cli::cli_abort(c(
      "The {.arg scale} function must have an {.arg aesthetics} argument.",
      i = "{.fn {scale_arg}} does not have that argument."
    ))
  }
  formals(scale)$aesthetics <- aesthetic
  scale
}

# Valid input
scale_fill_viridis_c <- reuse_scale(scale_colour_viridis_c, "fill")

# Bad input
reuse_scale(print, "fill")
#> Error in `reuse_scale()`:
#> ! The `scale` function must have an `aesthetics` argument.
#> ℹ `print()` does not have that argument.

reuse_scale("scale_fill_viridis", "colour")
#> Error in `reuse_scale()`:
#> ! `scale` must be a function, not the string "scale_fill_viridis".

^{Created on 2023-10-12 with reprex v2.0.2}

[steveharoz]

Yes! I've thinking about something similar but didn't know about fn_fmls_names.

Does that approach duplicate the documentation association? For example, if you copy scale_colour_hue() to scale_fill_hue, will the new function be linked to the same docs? Or would you need to specify that somewhere?

[teunbrand]

No you'd still have to document them as before. Quick boilerplate:

#' The foo scale
#'
#' @param aesthetics descriptions
#'
#' @return something
#' @export
#'
#' @examples
#' NULL
scale_foo_continuous <- function(aesthetics = "foo") {
  print(aesthetics)
}

#' @rdname scale_foo_continuous
#' @export
scale_bar_continuous <- reuse_scale(scale_foo_continuous, "bar")