Unified palette arg

[grantmcdermott]

Addresses the second part of #19 .

Some quick notes:

• I've left the default colour palette as Okabe-Ito for the moment to becnhmark against our existing test suite.
• I had to do some tricksy substitute footwork to get all eventualities covered, i.e. users can pass strings or functions, where we can still splice in the number of colours (groups) later on. Similarly, I also had to drop palette as an argument for plot2.formula and plot2.density and rather pass this through .... Otherwise substitute doesn’t work properly. Maybe there's a more failsafe way to do this?

Examples:

library(plot2)

plot2(
  Temp ~ Day | Month, airquality,
  type = "b", pch = 16
  )

plot2(
  Temp ~ Day | Month, airquality,
  type = "b", pch = 16,
  palette = "tableau 10"
  )

plot2(
  Temp ~ Day | Month, airquality,
  type = "b", pch = 16,
  palette = palette.colors(palette = "Tableau 10", alpha = 0.5)
  )

Custom / user-supplied palette generating functions are accepted too. The only caveat is that these functions must take "n" (= number of colors) as a leading argument. Note that users don't have to specify "n" manually. But we need this assumption (i.e., that "n" is the first argument) to fit in with our existing color allocating logic. Quick example, based on Vincent's recommendation of the khroma package.

plot2(
  Temp ~ Day | Month, airquality,
  type = "b", pch = 16,
  palette = khroma::color("muted")
  )

^{Created on 2023-04-17 with reprex v2.0.2}

[zeileis]

Looks good to me. I just saw the is.character() issue that you already fixed and that "R4" is still commented. (Edit: Ah, just saw your explanation, makes sense.)

(Disclaimer: I just read the code and didn't run it.)

[grantmcdermott]

Thanks @zeileis! Sorry, I was still busy adding additional context (and examples) before you responded. Everything seems to be running pretty well from my side. But please do kick the tyres and give the code some scrutiny.

[zeileis]

For the tricksy stuff and splicing in n: Personally, I find this confusing and wouldn't use it. But others will probably perceive this differently.

An alternative would be to simply allow the specification of more colors than ngrps and then just selecting ngrps colors from it:

• From something like palette.colors(palette = "Okabe-Ito", alpha = 0.5) you always get the maximum number of colors in the corresponding palette and you can simply select the first ngrps from it.
• The same is not possible for hcl.colors() but instead you could select a large number of colors and then select an equidistant subset from it or go via colorRampPalette() to interpolate the colors.
• Then only the question remains how to distinguish the case where you select the first ngrps colors and where you interpolate. One strategy could be to switch to the latter if the number of colors is greater than 36 because that is the largest qualitative palette I'm aware of. Also not ideal and potentially confusing but less "magical" than the evaluation tricks in my opinion.

[grantmcdermott]

Super, thanks for the feedback @zeileis. I hope to circle to this once I get some other things out of the queue. Let's hope Vincent finds a sec to take a look too, since I'm overwriting his by_col function.

[vincentarelbundock]

This looks like an excellent change! I wonder if it might not make sense to do some pre-processing on the palette argument before passing it to by_col. Passing through ... seems not great, so it might be worth having a bit of boilerplate duplication for a cleaner interface.

Sorry I don't have time to look at this in more detail now.

[zeileis]

Not much of an update here, I just wanted to show a brief illustration of what I thought could be a useful strategy for reducing a longer color vector to the ngrps needed.

For the qualitative palettes from palette.colors() we can simply select the first ngrps (below set to 5 for illustration). The these two are identical:

palette.colors(n = 5, palette = "Okabe-Ito")
##       black      orange     skyblue bluishgreen      yellow 
##   "#000000"   "#E69F00"   "#56B4E9"   "#009E73"   "#F0E442" 
palette.colors(palette = "Okabe-Ito")[1:5]
##       black      orange     skyblue bluishgreen      yellow 
##   "#000000"   "#E69F00"   "#56B4E9"   "#009E73"   "#F0E442" 

For hcl.colors() we can reduce the number of colors to ngrps (set to 12 for illustration below) by going through colorRampPalette(). This will yield very similar or in some cases even idential output to setting n = ... explicitly.

hcl.colors(12, palette = "Viridis")
##  [1] "#4B0055" "#45256B" "#30437F" "#005F8E" "#007896" "#009097" "#00A691"
##  [8] "#00B983" "#3AC96D" "#8BD650" "#C8DF32" "#FDE333"
colorRampPalette(hcl.colors(100, palette = "Viridis"))(12)
##  [1] "#4B0055" "#45256B" "#30437F" "#005E8E" "#007896" "#009097" "#00A591"
##  [8] "#00B883" "#3AC96D" "#8BD650" "#C8DF32" "#FDE333"

There are two caveats: First, colorRampPalette() drops the alpha channel so this would have to be re-added (if used). Second, we somehow need to distinguish the two cases. One heuristic could be the length of the color vector (as suggested in my previous comment). An alternative heuristic would be to set an attribute to the color vector so that we remember which function was used for generating the palette. Then we would know how to handle palette.colors and hcl.colors and only need to decide which strategy to use otherwise.

[grantmcdermott]

(Just a quick note to apologize for being slow with this: I was hoping to get to your comments during the week, but I had too much else going on. I'm heading out on vacation tomorrow, but will be back next week to finish this off.)

[vincentarelbundock]

Vacation should always be priority 1. Enjoy!

[grantmcdermott]

Hi folks. Again, sorry for keeping you both waiting on this.

So, after of thought, I've decided to merge this PR as-is. I don't mean to fob off the legitimate points and concerns that you have raised @zeileis (far from from it!). But I think there are two arguments for expediency:

1. The current implementation gives us user-level functionality that is close to optimal IMHO. I'm very happy for us to take another crack at the internals once this PR is merged, so please feel free to do so afterwards. On that note, Achim I like your colorRampPalette example/idea a lot and you're certainly better placed than am I to make the code work. One UI thing I'd like to avoid, however, is requiring users to guess n ahead of time, or (similarly) having to supply a large n as a placeholder that will ultimately get overwritten. Is there a way to achieve this without ultimately reverting to the substitute approach that I've had to use here?
2. The other reason is that I don't want to keep this PR from dragging on is to unblock @zeileis from the faceting / factor modules in #2. I'll ping that thread as soon as I'm done with some other little changes I want to make.

[zeileis]

Fair enough. I would still argue, though, that stopping if n does not match is not very user-friendly:

https://github.com/grantmcdermott/plot2/blob/main/R/by_aesthetics.R#L12-L14

And using colorRampPalette() for going from a palette with a fixed set of colors to a palette with the desired number of colors is something that is quite common, I think. This is what is done for all the palette from RColorBrewer because they just provide a fixed set anyways.

But as you said: We can also revisit this later.