Closed grantmcdermott closed 6 months ago
This looks fantastic, thanks for the work! A few minor comments:
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.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.Super nice implementation in general. Very nice feature!
Great, thanks for the feedback @vincentarelbundock!
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.)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)
.I'll try to action these suggestions when I get a sec and then we can merge unless anything else comes up.
Thanks, Grant @grantmcdermott, for adding this. I'll take a closer look later today or tomorrow!
Thanks both!
(@vincentarelbundock I've attempted to address all three of your comments. Please kick the tyres and see if anything unexpected happens.)
Looks and works great here!
Does defaulting to inches make people angry?
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.
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.
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 :-)
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.
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.
Wait, in that last example, the height and weight are standalone arguments?
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.
Wait, in that last example, the height and weight are standalone arguments?
Yes. I thought that was the request?
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.
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.
Closes #125.
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 maintinyplot(..., file(name) = X)
function later somehow.