Skip to content

Conversation

@dempz
Copy link
Contributor

@dempz dempz commented Dec 19, 2025

Fixes #65

@dempz dempz changed the title Save plot i65 New function for saving ggplots with default parameters Dec 19, 2025
@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 97.61905% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
R/thekids_save.R 97.61% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Contributor

@AstroRobin AstroRobin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the default for the path parameter be NULL or '.', the former is consistent with the default for ggsave? Note, I do see the reason to not have path = NULL because it is required to create the directory before saving plots. Perhaps instead, we should set the default to path='.', which avoids creating a directory unless explicitly asked for by the user.

Also for defaults, I feel like 'full landscape' will be the most common default for plots over 'full portrait'. I think the documentation should list all the possible layouts.

device documentation should list 'pdf' and 'png' — or even the full list provided in ggsave(). Technically ggsave() accepts device as either a string or alternatively as a device object like png(). I wonder if force converting device to character will break certain use cases. Counter argument to the above is that the png(...) like function also accept things like width and height, which may break the current layout in thekids_save.

if '.png' or '.pdf' are included in the filename, then the output file will look something like "test.pdf_fill_portrait.pdf". Can use tools::file_ext() and tools::file_path_sans_ext() to get the components separated.

On the point of adding e.g. '_full_portrait' to the end of output filenames, I feel like this is only useful if multiple formats are requested simultaneously (likely not the most common use case). So I suggest that if only a single output is being saved, just behave like the typical ggplot() save without adding the specifics of the layout.

Could save_params be renamed to layout_params or similar? To me save_params sounds like a function used to save parameters. Not precious about this, so happy to ignore.

Not really sure if this is just my machine, but I get errors when trying to save in 'pdf':

thekids_save(filename='testing.pdf')
Saving to: output/testing_full_portrait.pdf
Error in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y, :
invalid font type

The plot, although saving, does not have any labels.

@dempz
Copy link
Contributor Author

dempz commented Dec 21, 2025

Interesting re labels being omitted from plots... I cannot seem to reproduce this on my machine (where Barlow is installed) nor on my VM (where Barlow is not installed).

Changes (running tab)

  • Use tools::file_path_sans_ext() to properly treat file name.
  • Rename save_params -> layout_params
  • Default directory set to "."

Further Thinking Required

Omitting layout from file name unless multiple selected

I do think there is some utility here in how it's currently structured and it's nice being able to differentiate between differentiate the files based on layout from the outset. Specifically, I wonder about the use case where the function might be (later) used again with a different layout specified, which would trigger the overwrite warning.

Admissible Devices/Layout Dimension Parameters

Certainly agree with the sentiment here, and this was simply preserved from the previous function. Having the full suite of devices would be helpful, and I think devices could even default to NULL--where ggsave will simply guess the device based on the filename extension.

The layout dimensions are identical across the "pdf" and "png" devices---begging the question as to whether the device flag could simply be dropped. Perhaps simplifying and instead including a "page size" parameter (A4, A3, A5 etc.) could be more useful?

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.

Create plot save function with default sizes

3 participants