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

file extension parameter for ggsave() to allow device specification for files without extension #939

Merged
merged 1 commit into from Jun 19, 2015

Conversation

Projects
None yet
4 participants
@sebkopf
Contributor

sebkopf commented Apr 7, 2014

Thank you for the fantastic ggplots package!

This is a suggested implementation for specifying a file extension in ggsave to allow device matching (pdf, png, jpg, etc.) for filenames without extension. The suggestion is motivated by the problem of using ggsave in a shinyApp for letting the user download a plot (for example addressed on StackOverflow here: http://stackoverflow.com/questions/14810409/save-plots-made-in-a-shiny-app). Because the shiny server provides a temporary file without extension, ggsave can't match to any of the internally defined device functions. I'm not aware of a currently available solution to this problem and although there are several work-arounds, I like the simplicity of ggsave and wanted to suggest this solution (not sure if it's the best way to do it, but it's the most elegant I could think of). This would allow use of ggsave for a shinyApp downloadHandler like so:

# In ui.R:
downloadLink('downloadPlot', 'Download')

# In server.R:
output$downloadPlot <- downloadHandler(
  filename = "myplot.pdf",
  content = function(file) {
    ggsave(file, plot = qplot(speed, dist, data = cars), ext = "pdf")
  }
)

I'm relatively new to GitHub and this is my second contribution to someone else's project, please forgive if I'm missing some specific conventions or etiquette about forking/pull requests.

Sebastian

@BrianDiggs

This comment has been minimized.

Contributor

BrianDiggs commented Apr 8, 2014

What advantage does this approach have over passing this information as the device argument?

ggsave(file, plot = qplot(speed, dist, data=cars), device = pdf)
@sebkopf

This comment has been minimized.

Contributor

sebkopf commented Apr 8, 2014

Hi Brian,
I agree, that would be great if it worked that way but it doesn't have the desired effect. ggsave will call the passed in pdf function directly rather than the wrapper defined within ggsave. This works out okay for pdf because the only parameter ggsave sets with the wrapper is version = "1.4" (my bad for poor choice of pdf in the sample code, sorry). When you try the same with png or jpeg you run into trouble because now the ggsave wrapper does more:

ggsave(file, plot = qplot(speed, dist, data=cars), device = png)

will save a png with tiny width and height because the default units are "px" and ggsave passes the parameters in inches. Instead, a workaround requires specifying these parameters (res and units) in a separate wrapper (verbatim from ggsave):

png <- function(..., width, height) {
     grDevices::png(...,  width=width, height=height, res = 300, units = "in")
}
ggsave(file, plot = qplot(speed, dist, data=cars), device = png)

This works just fine and I'm using this approach myself but it's not very intuitive. I'm happy to keep using this workaround but wanted to suggest the above solution in case you guys would be interested in a more intuitive implementation.

Another way of doing it without introducing any extra parameters would be by allowing device = 'character' ('pdf', 'png', 'jpg', etc.) in addition to device = function(...) {} and simply adding this code block in ggsave:

 if (is.character(device)) {
      device <- match.fun(device)
 }

which would make this work directly:

ggsave(file, plot = qplot(speed, dist, data=cars), device = 'png')

What do you think?

@BrianDiggs

This comment has been minimized.

Contributor

BrianDiggs commented Apr 14, 2014

Thanks for the explanation; you are right that passing the device function misses the ggsave defaults that get set when the device is set via the defaults based on filename extension. I like the idea of allowing device to be a character argument which is then matched to the (internal, first) device functions better than the approach of adding a new parameter. But you can also wait to hear the opinion of an actual maintainer before making that change.

@hadley

This comment has been minimized.

Member

hadley commented Apr 22, 2014

  • Motivate the change in one paragraph, and include it in NEWS.
    In parentheses, reference your github user name and this issue:
    (@hadley, #1234)
  • Check pull request only includes relevant changes.
  • Use the official style.
  • Update documentation and re-run roxygen2
  • Add minimal example, if new graphical feature
@hadley

This comment has been minimized.

Member

hadley commented Apr 22, 2014

Looks good. Can you please do the last two things listed above?

@hadley hadley added the feature label Apr 22, 2014

@hadley hadley added this to the v1.0.0 milestone Apr 22, 2014

@sebkopf

This comment has been minimized.

Contributor

sebkopf commented Apr 22, 2014

Thanks Hadley, can you clarify real quick if you prefer the implementation discussed with Brian above allowing device as a character argument (that can be matched against the internally defined device functions) rather than the new file extension parameter?

I'm happy to implement the latter and file as a separate pull request (+NEWS) or stick with the current and include it in NEWS. Let me know, thanks.

@hadley

This comment has been minimized.

Member

hadley commented Apr 22, 2014

Ok, yeah, I'd prefer match.fun(device)

@hadley hadley modified the milestone: v1.0.0 Apr 23, 2014

@sebkopf

This comment has been minimized.

Contributor

sebkopf commented Apr 24, 2014

Sounds good, I implemented it with match.fun(device) instead, added an example in @examples and a line in the NEWS. Please let me know if there's anything else missing.

Update: the Travis CI build is failing but it appears to be because of a syntax error elsewhere, not in this pull request (see #949 for reference)

@JHonaker

This comment has been minimized.

JHonaker commented May 23, 2014

I'm not really sure how to resubmit this pull request for Travis CI, but it should build fine now. #949 fixed the error.

@sebkopf

This comment has been minimized.

Contributor

sebkopf commented May 30, 2014

Yeah, I didn't know how to do that (resubmit the request for Travis), it builds just fine for me with #949 fixed. I'm happy to resubmit but the only way I can think of doing that is closing this pull request and creating a new one - is that the recommended approach?

@BrianDiggs

This comment has been minimized.

Contributor

BrianDiggs commented May 31, 2014

According to this: http://stackoverflow.com/q/17606874/892313 it should be possible to trigger a new attempt at the build. But maybe Hadley (or one of the repo owners) has to do it because I don't see the triggering icon there.

@sebkopf

This comment has been minimized.

Contributor

sebkopf commented Sep 2, 2014

Just a quick follow-up, I think this feature could still be very useful to users judging from the traffic that the original post on stack overflow still receives (save-plots-made-in-a-shiny-app).

As far as I can tell, all that's missing is a retriggering of the Travis CI build here by a repo owner. @hadley could you help out with this?

Alternatively, I'm happy to close this pull request and create a new one for this feature if that's easier, let me know.

R/save.r Outdated
if(!exists(ext, mode = "function")) {
stop("No graphics device defined for the file extension '", ext, "'. ",
"Make sure to specify a filename with supported extension or ",
"set the device parameter.")

This comment has been minimized.

@hadley

hadley Jun 11, 2015

Member

Can you please add call. = FALSE here?

@hadley

This comment has been minimized.

Member

hadley commented Jun 11, 2015

Just update this PR by merging/rebasing against master, and re-pushing the branch to github.

@hadley

This comment has been minimized.

Member

hadley commented Jun 18, 2015

Are you still interested in this PR?

@sebkopf sebkopf force-pushed the sebkopf:master branch from 8149ae2 to d5b77f7 Jun 19, 2015

device parameter supports character argument
Parameter `device` now supports character argument to specify which
supported
  device to use ('pdf', 'png', 'jpeg', etc.), for when it cannot be
correctly
  inferred from the file extension (for example when a temporay
filename is
  supplied server side in shiny apps)

@sebkopf sebkopf force-pushed the sebkopf:master branch from d5b77f7 to 1c50c53 Jun 19, 2015

@sebkopf

This comment has been minimized.

Contributor

sebkopf commented Jun 19, 2015

Okay, should be all ready to go now (including the requested .call = FALSE addition and some commit cleanup). Thanks for the reminder msg, missed last week's while traveling. And thanks a lot for the rebasing hint to retrigger Travis CI build, that's exactly what I was missing - learned something new again.

hadley added a commit that referenced this pull request Jun 19, 2015

Merge pull request #939 from sebkopf/master
file extension parameter for ggsave() to allow device specification for files without extension

@hadley hadley merged commit 3273c5a into tidyverse:master Jun 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hadley

This comment has been minimized.

Member

hadley commented Jun 19, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment