tinyplot icon indicating copy to clipboard operation
tinyplot copied to clipboard

grid = grid() not working properly with faceted plots

Open grantmcdermott opened this issue 1 year ago • 4 comments

Specifically, it only seems to be working for the first facet. Tweaked example from the README, where we use grid = grid() instead of grid = TRUE.

library(tinyplot)
plt(
  Sepal.Length ~ Petal.Length | Sepal.Length, data = iris,
  facet = ~Species, facet.args = list(bg = "grey90"),
  pch = 19,
  main = "Faceted Species!",
  grid = grid(), # <--
  frame = FALSE
)

Created on 2024-08-04 with reprex v2.1.1

grantmcdermott avatar Aug 04 '24 21:08 grantmcdermott

Not 100%, but there's a chance this is my fault.

The grid argument must be passed explicitly to the helper functions I have modularized. When they are passed via ... it gets evaluated at an undesirable time.

vincentarelbundock avatar Aug 04 '24 21:08 vincentarelbundock

Thanks, don't stress. It's not a major bug (and may well have been there for ages). But still worth fixing when we get a sec.

grantmcdermott avatar Aug 04 '24 21:08 grantmcdermott

I looked into this as part of #207, but it was trickier to fix than I had time for.

The problem is here... https://github.com/grantmcdermott/tinyplot/blob/e5ac2b634006fea3e57e7ea3f962c4018cf00931/R/facet.R#L396 ... since grid gets evaluated during the is.null() check, which in turn ends up (re)assigning the values of the grid argument to a list of atx and aty values (as per grid). So, we're basically dealing with some unhelpful combination of NSE, promises, and argument/function overloading.

I tried various things, including using !exists("grid") instead of is.null(grid), as well as deparse1(substitute(grid)), but I couldn't get it to work.

Maybe someone else wants to take a stab?

grantmcdermott avatar Aug 26 '24 03:08 grantmcdermott

I don't think that there is a "good" workaround which we could apply only in the draw_facet_window() function. If there is, it would probably be rather fragile. Hence, I see two routes which we could consider - but I haven't thought them through! I just wanted to mention them here in case it is helpful for the discussion. If not, that's also ok.

  • substitute() earlier.
    This would be similar to what we do for palette= and legend= for which we just substitute() (without deparse()) early on in tinyplot.default() and then later on essentially do eval(str2lang(...), ...) in the auxiliary functions.
  • Introduce a helper function like tinygrid().
    Then we could make tinygrid() a function that processes its arguments and returns a function $grid() (among other elements if needed) that we can then evaluate later on.

zeileis avatar Aug 26 '24 09:08 zeileis