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

Par restore #13

Merged
merged 10 commits into from
Apr 10, 2023
Merged

Par restore #13

merged 10 commits into from
Apr 10, 2023

Conversation

grantmcdermott
Copy link
Owner

@grantmcdermott grantmcdermott commented Apr 10, 2023

Fixes #6 by adding a par_restore argument.

Defaults to par_restore = FALSE, in which case any changes to par during a plot2() call (e.g., from a legend repositioning) will persist afterwards. I initially resisted persisting changes as a default, but in hindsight I think this is more consistent with the normal base plot workflow. (And, in the case of #6, means that we can add elements after a plot is drawn in the regular manner.) Ofc users can opt out of this behaviour by switching to par_restore=TRUE.

Examples:

library(plot2)

op = par(no.readonly = TRUE)

plot2(Sepal.Width ~ Sepal.Length | Species, iris, grid = grid())
points(6,3, pch = 17, col = "red", cex = 1.5)

plot2(
  mpg ~ wt | cyl, mtcars,
  pch = 19,
  grid = grid(),
  legend.position = "right!",
  legend.args = list(title = "How many cylnders do you have?")
  )
lines(lowess(mtcars[["wt"]], mtcars[["mpg"]]), col = "red")

# Caution: Note that adjust margin persists b/c we didn't specify 
plot(1:10)

par(op)

# Try again with par_restore = TRUE
plot2(
  mpg ~ wt | cyl, mtcars,
  pch = 19,
  grid = grid(),
  legend.position = "right!",
  legend.args = list(title = "How many cylnders do you have?"),
  par_restore = TRUE
)

# Adding elements to the plot won't work properly now...
#lines(lowess(mtcars[["wt"]], mtcars[["mpg"]]), col = "red")

# But our normal defaults have been preserved
plot(1:10)

Created on 2023-04-09 with reprex v2.0.2

@grantmcdermott
Copy link
Owner Author

grantmcdermott commented Apr 10, 2023

@vincentarelbundock I can't get these tinysnapshot tests to pass on GitHub's CI even though I generated them locally using Ubuntu 22.04 on a Rocker image. (As mentioned before, my default Arch system generates slight differences in fonts that also triggers errors.)

I can switch to MacOS, which I have on my work machine. But I'm wondering what the best long-term solution, is given that you and (I think?) @zeileis are on Ubuntu. Any thoughts?

@zeileis
Copy link
Collaborator

zeileis commented Apr 10, 2023

Just fyi: I'm on Debian testing. Not much I can contribute off the top of my head otherwise. Just maybe that there is the gdiff package from Paul Murrell. I've never worked with it, though.

@vincentarelbundock
Copy link
Collaborator

vincentarelbundock commented Apr 10, 2023

@zeileis tinysnaphot uses a very similar approach to gdiff, based on magick.

@grantmcdermott I added a new argument to tinysnapshot which allows us to set the font. Maybe that will help? I pushed a commit to this PR. Can you try this:

  1. Make sure Liberation font is installed on your Rocker. Maybe: sudo apt install fonts-liberation
  2. Install the dev version of tinysnapshot: remotes::install_github("vincentarelbundock/tinysnapshot")
  3. Pull from this updated PR.
  4. Run the test suite locally. tinytest::run_test_dir("inst/tinytest")

The snapshots were re-generated on my computer, and they match the Ubuntu from Github actions. Now I need to figure out if this will work on your local install.

@grantmcdermott
Copy link
Owner Author

Great, thanks for taking a look. I'm a bit pushed for time this morning. So let me merge this in a bid for expediency and then I'll test with Liberation when I get a chance later.

@grantmcdermott grantmcdermott merged commit 286a5f5 into main Apr 10, 2023
5 checks passed
@grantmcdermott
Copy link
Owner Author

Tested quickly and good news: explicitly requiring Liberation fonts fixes the problem (and on my native Arch system too).

Huzzah!

@zeileis
Copy link
Collaborator

zeileis commented Apr 10, 2023

Thanks, Vincent @vincentarelbundock, I didn't realize that tinysnapshot was yours!

Will try to use it for our topmodels package as well, this might resolve some problems we had with our current hand-crafted solution.

@vincentarelbundock
Copy link
Collaborator

@zeileis Yeah, this is a very very new package. So fair warning: there may (will?) be some rough corners. But I'm always eager to get feedback from early adopters!

Just make sure you read this section of the README on "deterministic" plots, and maybe wait a few days for version 0.0.3 to come out.

@grantmcdermott grantmcdermott deleted the par_restore branch January 18, 2024 19:48
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

Successfully merging this pull request may close these issues.

Unable to add further elements on the plot2() plot.
3 participants