Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grid = grid() not working properly with faceted plots #193

Open
grantmcdermott opened this issue Aug 4, 2024 · 4 comments
Open

grid = grid() not working properly with faceted plots #193

grantmcdermott opened this issue Aug 4, 2024 · 4 comments

Comments

@grantmcdermott
Copy link
Owner

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

@vincentarelbundock
Copy link
Collaborator

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.

@grantmcdermott
Copy link
Owner Author

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
Copy link
Owner Author

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

The problem is here...

if (is.null(grid)) grid = .tpar[["grid"]]

... 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?

@zeileis
Copy link
Collaborator

zeileis commented Aug 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants