allow palette.qualitative specification via function

[zeileis]

Fixes #593

In order to support function specifications of palette.qualitative in themes, I think that we only need to make sure that match_palette_name(theme_palette, ...) is only used if theme_palette is a character string (but not a function).

Then both palette.qualitative and palette.sequential can be a function in a theme:

my_pal <- colorRampPalette(c("red", "yellow", "blue"))
tinytheme("clean2", palette.qualitative = my_pal, palette.sequential = my_pal)
tinyplot(1:10, by = factor(1:10)) ## qualitative

tinyplot(1:10, by = 1:10) ## sequential

[grantmcdermott]

Thanks @zeileis. I did some testing and it mostly looks good. The one exception was for passing palette.sequential as part of a list argument to an ephemeral theme.

tinyplot(
  1:10,
  by = 1:10,
  theme = list(palette.sequential = my_pal)
)

Would it work to change l. 304 here

tinyplot/R/by_aesthetics.R

Line 304 in 4ef61ae

pal_theme = get_tpar("palette.qualitative", default = NULL)

to

  pal_theme = if (ordered || gradient) {
    get_tpar("palette.sequential", default = NULL) %||% get_tpar("palette.qualitative", default = NULL)
  } else {
    get_tpar("palette.qualitative", default = NULL)
  }

?

(I haven't tested)

[zeileis]

Makes sense. I have omitted the %||% addition, though. Then the fallback is handled in resolve_palette_colors().

Should we add the analogous computation in by_bg()?

[grantmcdermott]

Should we add the analogous computation in by_bg()?

Good call. I can't think of a reason not to.

[grantmcdermott]

@zeileis once the by_bg component has been added, could you add some shapshot tests? Happy to merge after that.

[zeileis]

I added the same palette handling in by_bg() as in by_col(). But my impression is that this wouldn't have been necessary due to the way fallbacks are handled. At least I wasn't able to create an example where the old bg handling failed.

Also added some tinytests/snapshots.

[grantmcdermott]

At least I wasn't able to create an example where the old bg handling failed.

Thanks. If memory serves, this example was failing earlier but is working now.

my_pal = colorRampPalette(c("red", "yellow", "blue"))
i = 4*(0:10)
tinyplot(
  xmin = 100+i, ymin = 300+i, xmax = 150+i, ymax = 380+i,
  by = i, fill = 0.2,
  type = "rect",
  theme = list("default", palette.sequential = my_pal)
)

Either way, agree that having some backup/redundancy doesn't hurt.

P.S. I'm not sure why the added snapshot tests are failing on the CI. I just tested locally and everything is passing, so it might be an annoying false positive case. I have to step out for a bit, but can investigate when I"m back.

[grantmcdermott]

Figured it out; just an innocuous main branch difference.

[zeileis]

Excellent, thanks for the example and for fixing the CI failure! 💪