grantmcdermott / tinyplot

Lightweight extension of the base R graphics system
https://grantmcdermott.com/tinyplot
Apache License 2.0
211 stars 7 forks source link

API change: single `legend` and `palette` arguments #19

Closed grantmcdermott closed 1 year ago

grantmcdermott commented 1 year ago

TL;DR Roll the legend.position and legend.args arguments into a single legend = legend(...) / legend = <keyword> argument. Similarly, roll the palette and palette.args arguments into a single palette = <function> / palette = <keyword> argument.

Problem

An API/design decision I'd like to nail down before a CRAN submission is the best way to handle the legend and palette arguments. At the moment, each of these is broken into two user-facing arguments. Respectively:

The current setup has some advantages, but I don't really like that we require two separate arguments for each "aspect". Other concerns include the fact that we don't use all arguments available to, say, legend creation and existing arguments don't necessarily align with their base equivalents. (An example: the "title" argument in legend.args is equivalent to the "legend" argument in the base legend() function.) Finally, the current API also places some constraints on user-specified inputs. For example, if a user wanted to supply a bespoke colour-generating palette as a function.

Proposal

An alternative would be to roll each of these into a single argument that accepts a function (and possibly a string for a convenient shorthand to change a leading/key feature). Specifically:

Decision?

Personally I'm leaning towards the proposed setup, despite be a breaking change. I would love to hear other opinions before putting in a PR, though.

vincentarelbundock commented 1 year ago

That all sounds good to me. This is the perfect time for breaking changes.

zeileis commented 1 year ago

+1

zeileis commented 1 year ago

Talking about breaking changes: Is everyone still happy with the name plot2()? I think it's not a great name but I'm not sure whether I have anything better. I just thought I would mention my doubts now rather than after a CRAN release.

A somewhat longer but more verbose alternative might be baseplot(). Short alternatives would be draw() or viz(). Maybe someone else has better ideas. Currently, I think I slightly prefer baseplot() over plot2().

Happy to make this a separate issue if you think it is worthwhile to discuss.

grantmcdermott commented 1 year ago

Happy to make this a separate issue if you think it is worthwhile to discuss.

Bedtime here, but yes please role into a separate issue and we can discuss.

zeileis commented 1 year ago

Done now: https://github.com/grantmcdermott/plot2/issues/22

grantmcdermott commented 1 year ago

Another thing to think about while we're at it: Should the default palette for small groups still be Okabe-Ito? I think it's an elegant palette, but I'm a bit worried the yellow (fifth group) is hard to see with standard par settings (esp. the pure white plot background). We might think of defaulting to the regular R (>=4.0) palette() instead?

vincentarelbundock commented 1 year ago

I like the idea of being as close to base as possible, while allowing easy switch. So the 4.0.0 base palette feels like a nice choice.

One palette I've been using recently is Tol's: https://cran.r-project.org/web/packages/khroma/vignettes/tol.html

zeileis commented 1 year ago

I agree, consistency with base R would be a nice touch and now it's so easy to switch.

Just for the record: I've had a similar discussion with Deepayan (Sarkar) recently and he was nice enough to incorporate the outcome of our discussions into the most recent lattice version (0.21-8). It now uses the following defaults:

I think that none of this is directly relevant for plot2 at the moment. But maybe it becomes useful at some point.

grantmcdermott commented 1 year ago

Super helpful, thanks for the feedback both.

I'll priortise this as the next PR and then I think we should release the next (dev) version, given the breaking change. Will obviously have to update all of the snapshots in our test suite too.

PS. @zeileis Thanks for the HU on lattice. That's cool! Like lattice (trellis.par.set) and ggplot2 (e.g., getOption("ggplot2.discrete.fill")) I'd ultimately like to allow users to control plot2 default palettes via a global option. I'll see if I can work that in...