Skip to content

Support file arg for writing plots to disk#143

Merged
grantmcdermott merged 15 commits intomainfrom
filename
Apr 5, 2024
Merged

Support file arg for writing plots to disk#143
grantmcdermott merged 15 commits intomainfrom
filename

Conversation

@grantmcdermott
Copy link
Copy Markdown
Owner

Closes #125.

tinyplot(Sepal.Length ~ Petal.Length | Species, data = iris, file = "~/Desktop/myplot.png")
tinyplot(Sepal.Length ~ Petal.Length | Species, data = iris, file = "~/Desktop/myplot.pdf")
# etc

Right now, the functionality is pretty simple: It just detects the file extension (where only ".png", ".pdf", or ".svg" are supported) and invokes the appropriate device type. Users can also control some basic elements of the output size via tpar("file.width", "file.height", "file.res"), where the defaults are 8, 5, and 300 respectively. We might think about integrating these into the main tinyplot(..., file(name) = X) function later somehow.

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

This looks fantastic, thanks for the work! A few minor comments:

  1. Drop the file alias. There are already a ton of arguments in the function, which makes the documentation hard to read. Adding aliases just pollutes the ? docs and requires me to figure out what the difference is between them.
  2. I wonder if we should add width and height arguments. I expect to be using those every single time I save to file, so I feel like it makes sense to have explicit argument. Resolution is probably something you want to be consistent across all files, so you don't need it in the main function. After width and height, I'd give a hard "NO" to any other graphics device option feature request.
  3. Is there a way to block the R Graphics window? On my machine, when I save to file I get a blank window that pops up. It's a bit confusing and annoying.

Super nice implementation in general. Very nice feature!

@grantmcdermott
Copy link
Copy Markdown
Owner Author

grantmcdermott commented Mar 28, 2024

Great, thanks for the feedback @vincentarelbundock!

  1. Cool. Let's go with file instead of filename then since it's shorter. (Annoyingly, pdf(), png(), etc. aren't consistent here, so we shouldn't fee beholden to any one thing.)
  2. Again, cool. I think I could make it a pretty simple implementation where file should either be a character string (denoting the file path) or a list containing the arguments you mentioned file = list(path = x, width = y, height = z).
  3. Hmmm, not sure off the top of my head. I need to step through the function to figure out where this is coming from.

I'll try to action these suggestions when I get a sec and then we can merge unless anything else comes up.

@zeileis
Copy link
Copy Markdown
Collaborator

zeileis commented Mar 29, 2024

Thanks, Grant @grantmcdermott, for adding this. I'll take a closer look later today or tomorrow!

@grantmcdermott
Copy link
Copy Markdown
Owner Author

Thanks both!

(@vincentarelbundock I've attempted to address all three of your comments. Please kick the tyres and see if anything unexpected happens.)

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

Looks and works great here!

Does defaulting to inches make people angry?

@zeileis
Copy link
Copy Markdown
Collaborator

zeileis commented Mar 30, 2024

Thanks, again, Grant. I'm still afraid that it will turn into a can of worms but I also agree that this is a nifty feature that I will use myself. Some comments:

  • Inches: Defaulting to inches might make people angry but it's what they also have to do everywhere else (e.g., in knitr, quarto, etc.).

  • Default height/width: I'm not sure whether a non-square default is the best choice. In base R but also in knitr etc. the defaults are (almost?) all square. So that would feel most natural - and least surprising - to me.

  • Setting height/width: I agree that I will use this all the time, so setting it conveniently would be a big plus. Maybe it's worth making these standard arguments? This might be useful for interactive plotting on the screen as well. For example

    tinyplot(Petal.Length ~ Petal.Width | Species, data = iris, facet = "by", height = 4.5, width = 12)
    

    could internally call

    dev.new(height = 4.5, width = 12)
    tinyplot(Petal.Length ~ Petal.Width | Species, data = iris, facet = "by")
    
  • Supported devices: I think we should add jpeg() to the list of supported devices for file extensions .jpg and .jpeg. This is easily available in base R and might be useful in some contexts (e.g., when I quickly want to get something without transparency).

  • Calling png: I would change line https://github.com/grantmcdermott/tinyplot/blob/filename/R/tinyplot.R#L460 as follows:

    png = png(filepath, width = filewidth, height = fileheight, res = fileres, units = "in"),
    

    The resulting file will be the same but I think that the code is more readable and more similar to the other devices.

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

FWIW, I agree on adding JPG and on the idea that separate arguments for height and width are probably a clearer interface than a named list. I get that we are trying to minimize the number of arguments, but this is not a common pattern and requires careful reading of the docs, whereas different arguments are easy to understand.

@grantmcdermott
Copy link
Copy Markdown
Owner Author

Excellent, thanks for the additional suggestions. I'll add jpeg support and tweak the png (and I guess jpeg?) code as you suggest, @zeileis.

I agree that inches won't please everyone. But I think it's the most common default nowadays and folks can always revert back to manual pdf() etc. if they want more control beyond this convenience feature.

I'm finding myself a little more reticent to switch to a default square layout since I never use this myself. But I think you're correct that this is the most common default, so it probably makes sense.

I'm more hesitant about the separate width and height args. I'd prefer to avoid starting up new (interactive) devices as a side effect, since I think this is going to be unexpected behaviour for most users who are going through an IDE like RStudio or VS Code. (I suppose we could use some control flow to avoid this unless height and width are called explicitly, but that requires its own documenting and expectation management.) I'm also unsure how this would or should interact within, say, a Quarto document. I have loooong flight back to the US coming up soon, so will have plenty of time to think about it :-)

@zeileis
Copy link
Copy Markdown
Collaborator

zeileis commented Mar 30, 2024

Just for clarification: I was suggesting only that when height and/or width are set but file is not that then we open a dev.new(). Thus, by default, we wouldn't open a new device, only when the user asks for a specific height/width.

We could also try to modify the existing device but I don't think that this is always possible. Also, I'm not sure how to accomplish a specific height/width in the RStudio plots window. If you have any pointers for this, I can try to have a look though. I'll also try out what happens in knitr/quarto.

@grantmcdermott grantmcdermott changed the title Support file(name) arg for writing plots to disk Support file arg for writing plots to disk Apr 1, 2024
@grantmcdermott
Copy link
Copy Markdown
Owner Author

grantmcdermott commented Apr 5, 2024

Okay folks, all your suggestions have now been added, including separate width and height args. Per Achim's request, these trigger a new interactive window (of the appropriate dimensions) if a corresponding file argument is not provided. It works well if you're using a free-standing session (e.g. terminal/vim/etc.), but less well if calling from an IDE with an integrated graphics window. I've added a caveat in the docs.

tinyplot(Petal.Length ~ Petal.Width | Species, data = iris, file = "~/myplot.jpg") # default 7x7
tinyplot(Petal.Length ~ Petal.Width | Species, data = iris, file = "~/myplot2.jpg", height = 4.5, width = 12) # override dimensions
tinyplot(Petal.Length ~ Petal.Width | Species, data = iris, height = 4.5, width = 12) # opens new interactive window

I think that this is now ready to merge, but either of you should feel free to test one more time and merge once you're happy.

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

Wait, in that last example, the height and weight are standalone arguments?

@grantmcdermott
Copy link
Copy Markdown
Owner Author

grantmcdermott commented Apr 5, 2024

Ugh, hold on. Testing from a different IDE (VS Code) is giving me annoying warning about calling par without a plot. I need to investigate further.

UPDATE: It's related to facets. I'll push a fix later today when I get time.

@grantmcdermott
Copy link
Copy Markdown
Owner Author

Wait, in that last example, the height and weight are standalone arguments?

Yes. I thought that was the request?

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

Ah no, sorry, I misread: saw the last example correctly, but thought the first two were still using the old named list specification.

My eyes are getting too old for small font print.

I think this is super nice! Don't wait for me to merge.

@grantmcdermott
Copy link
Copy Markdown
Owner Author

Minor hiccup fixed, so I'll go ahead and merge. @zeileis please feel to raise an issue if you catch something else post hoc.

Thanks again both for the great feedback and suggestions.

@grantmcdermott grantmcdermott merged commit 64626e4 into main Apr 5, 2024
@grantmcdermott grantmcdermott deleted the filename branch January 22, 2025 17:34
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.

Plot writing / saving argument

3 participants