WIP: set common scale parameters outside of the scale function

[clauswilke]

Here is an attempt at fixing #4269. The PR creates a new scales_params object type that can be added to ggplot2 objects to collect parameter values that will be added to scales at plot build time. In this way, we can set properties such as expansion or breaks or labels without knowing which scales object is being used for which aesthetic.

Reprex:
library(ggplot2)

ggplot(mtcars, aes(disp, mpg, color = hp)) + geom_point() +
  ggplot2:::scales_params("x", breaks = c(111, 222, 333, 444), name = "hello world") +
  ggplot2:::scales_params("colour", name = "test") +
  ggplot2:::scales_params("x", name = "displacement") +
  ggplot2:::scales_params("y", name = "fuel efficiency", expand = expansion(add = 10)) +
  scale_y_continuous(name = "test", expand = expansion(add = 0)) +
  ggplot2:::scales_params("y", breaks = c(15, 25, 35))

^{Created on 2020-11-19 by the reprex package (v0.3.0)}

One current downside is that parameters directly provided to a scale object have lower priority than parameters added previously (see the y expansion in the example). This can be fixed but will add a bit of complication and clumsiness. Not sure if it's worth it but happy to receive feedback.

@yutannihilation @thomasp85 @hadley Any thoughts?

[clauswilke]

Note to self: The way to fix the priority issue is to add a scales_params slot to scales objects and insert the contents of that slot into scales$scales_params when calling scales$add(). One additional complication this creates is that these parameters need to be removed if a scale is overwritten with a new scale object, since we wouldn't want parameters set for an earlier scale to stick around.

[yutannihilation]

Looks fine to me.

For the interface, I prefer something in scale_<aes>_...()-form for tab-compatibility. For example, the code below seems shorter than scales_params().

ggplot(mtcars, aes(disp, mpg, color = hp)) +
  geom_point() +
  scales_x_auto("hello world", breaks = c(111, 222, 333, 444)) +
  scales_colour_auto("test")

However, as it's not really a scale, the naming might be a bit confusing. So, I'm not confident if this is better than yours.

# Will the color scale of this be viridis? Or should it fall back to the default?
ggplot(mtcars, aes(disp, mpg, color = hp)) +
  geom_point() +
  scale_colour_viridis_c(option = "C") +
  scale_colour_auto()

For the implementation, what about creating a placeholder Scale so that we can merge it with the previous scale here?

ggplot2/R/scales-.r

Lines 20 to 37 in b76fa96

	add = function(self, scale) {
	if (is.null(scale)) {
	return()
	}
	prev_aes <- self$find(scale$aesthetics)
	if (any(prev_aes)) {
	# Get only the first aesthetic name in the returned vector -- it can
	# sometimes be c("x", "xmin", "xmax", ....)
	scalename <- self$scales[prev_aes][[1]]$aesthetics[1]
	message_wrap("Scale for '", scalename,
	"' is already present. Adding another scale for '", scalename,
	"', which will replace the existing scale.")
	}
	# Remove old scale for this aesthetic (if it exists)
	self$scales <- c(self$scales[!prev_aes], list(scale))
	},

Something like:

    if (any(prev_aes)) {
      if (is_placeholder_scale(scale)) {
        prev_aes$update_params(scale$params)
        return()
      }
    ...

[clauswilke]

I hadn't thought about the interface yet. scales_params() is meant as an internal function that is called by other functions. E.g. we already have xlab(), ylab(), and labs().

Not sure I understand the idea of the placeholder scale. Are you suggesting to use a new type of Scale object instead of the scales_params object I introduced?

[yutannihilation]

I hadn't thought about the interface yet.

Oh, I see.

Are you suggesting to use a new type of Scale object instead of the scales_params object I introduced?

Yes, I don't know yet if my approach is even doable, but it might be a choice. I'll try to make a quick implemention so that we can discuss it further...

[clauswilke]

I'll try to make a quick implemention so that we can discuss it further...

Sounds good. I'm not wedded to my approach, it just seemed the most straightforward to me.

[yutannihilation]

Okay, here's my proof-of-concept implementation (only for continuous x/y scales): master...yutannihilation:poc/pr-4271-parameter-only-scale

The basic idea is that if we treat a scale parameter object as a Scale, we can process it sequentially in ScalesList, which might solve the issue you described a bit easier:

One additional complication this creates is that these parameters need to be removed if a scale is overwritten with a new scale object, since we wouldn't want parameters set for an earlier scale to stick around.

But, I found my approach is not very clean. ScalesList$add() needs to distinguish the case when a scale is added by users and when a default one is added on ggplot_build(); the params should be cleared in the former case, while they should be applied to the default one in the latter case...

---

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

p <- ggplot(mtcars, aes(disp, mpg, color = hp)) +
  geom_point() +
  scale_x_auto("hello world", breaks = c(111, 222, 333, 444)) +
  scale_x_auto("displacement") +
  scale_y_auto("fuel efficiency", expand = expansion(add = 50))

p

# Parameters are ignored if a new scale is added 
p +
  scale_y_continuous("test")

^{Created on 2020-11-22 by the reprex package (v0.3.0)}

[clauswilke]

Your approach does have the benefit though of not introducing yet another object type with separate logic. Is there anything in the current two proof-of-concept implementations that differs in terms of features? Something my implementation can do that yours can't or vice versa?

To handle the cases correctly when explicit scale functions are added, we may have to start adding code to the scale constructors that checks whether arguments are missing or not, and keeps track of which parameters should be replaced based on that. This is the same issue for both approaches.

[clauswilke]

I think the scale_<aes>_...() naming can work, I just don't like auto (since that's a word that implies magic but doesn't explain anything). How about something like scale_<aes>_params() or scale_<aes>_settings() or scale_<aes>_adjust() to highlight that this is meant to modify a scale that is otherwise present already.

ggplot(mtcars, aes(disp, mpg, color = hp)) +
  geom_point() +
  scale_colour_viridis_c(option = "C") +
  scale_colour_settings(name = "horse power")

[clauswilke]

The advantage of this approach would be that once we move to a place where scales are settable via themes, the code looks clean again:

ggplot(mtcars, aes(disp, mpg, color = hp)) +
  geom_point() +
  scale_x_settings(limits = c(100, 300)) +
  scale_colour_settings(name = "horse power") +
  theme_viridis(option = "C")

[clauswilke]

It will just be a bit confusing which parameters can and cannot be set via scale_<aes>_settings(), unless we allow all parameters to be set in this way.

[yutannihilation]

Your approach does have the benefit though of not introducing yet another object type with separate logic.

Yes. To be fair, this might impose developers some additional care about different types of Scale, on the other hand.

I don't find any feature differences yet.

I just don't like auto (since that's a word that implies magic but doesn't explain anything).

I picked auto to imply that the scale type stays the default, which will be automatically chosen, in the assumption that it's the most common case to use it with the default ones. But I agree that we need some better name. I like adjust.

It will just be a bit confusing which parameters can and cannot be set via scale_<aes>_settings(), unless we allow all parameters to be set in this way.

Agreed. For example, if I were a user, I might wonder if I could tweak even the type of the scale:

p +
  scale_colour_settings(type = "viridis")

This is another reason why I chose auto so that it looks like a standalone scale that accepts only the limited numbers of parameters.

[teunbrand]

I think the idea to set parameters of scales without committing to any particular one is great. Is there anything I could do to help get this idea moving?

[teunbrand]

Hi @clauswilke and @yutannihilation, I'm really liking the direction of this PR. Is there any way in which I could be of any assistance? Apologies for the reminder.

[clauswilke]

@teunbrand I think the first question is whether @yutannihilation's approach or my approach is preferable. I don't have a strong opinion one way or another and am not actively thinking about this right now. From my perspective it would be best if you just started your own PR, taking whichever ideas/code from me are useful.

[yutannihilation]

I don't have a strong opinion one way or another and am not actively thinking about this right now

Same here!