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

Change in ggsave behaviour #4513

Closed
ecorreig opened this issue Jun 16, 2021 · 12 comments · Fixed by #4518
Closed

Change in ggsave behaviour #4513

ecorreig opened this issue Jun 16, 2021 · 12 comments · Fixed by #4518

Comments

@ecorreig
Copy link

I used to "add" ggsave("something.png") to a ggplot object but this has stopped working after version 3.3.3. In case it's been decided that this "feature" wasn't a good idea and the removal was on purpose, please better document that change (even if the possibility to do + ggsave() wasn't documented before; I think many people used it because it might the pipes very elegant).

In short, this worked on 3.3.3:

library(ggplot2)
data.frame(a = rnorm(100), b = rnorm(100)) |> 
  ggplot(aes(a, b)) +
  ggsave("temp.png")

But throws this error on 3.3.4:

Error: Can't add `ggsave("temp.png")` to a ggplot object.
@clauswilke
Copy link
Member

clauswilke commented Jun 16, 2021

Honestly, I've never seen anybody do this, and it's definitely not a supported feature to my knowledge. The only reason that it worked is probably that ggsave() returned NULL, and you can add anything to a plot if it returns NULL. But that doesn't mean it's sensible.

You were relying on the fact that ggsave() uses the last plot that was made if none is provided as an argument, and somehow it found the plot that you just generated. (This could just as well have gone wrong and you would have gotten one plot prior.)

The valid way to do what you want to do is this code:

library(ggplot2)
data.frame(a = rnorm(100), b = rnorm(100)) |> 
  ggplot(aes(a, b))
ggsave("temp.png")

This is the commit that caused the change in behavior: 23baab0

@thomasp85
Copy link
Member

Well, that certainly never was a feature — I'm surprised it ever worked in the first place... it is hard to anticipate when someone's hack stops to work which is also why it hasn't been communicated.

I don't suspect many have used this hack based on the fact that this is the first time I've seen it, but I could be wrong

@ecorreig
Copy link
Author

Honestly I never thought I was doing a hack, but I understand this was the case.

But still, with the new changes in R 4.1 and ggplot 3.3.4 I can do:

data.frame(a = rnorm(100), b = rnorm(100)) |> 
  ggplot(aes(a, b)) +
  geom_point() |>  
  ggsave(file = "temp.png")

But the mixing of pipes and + is prone to errors, so I don't know if it would be a good idea to add a feature of "adding" to ggsave.

You find more information about what I mean in the response I got in stackoverflow.

@yutannihilation
Copy link
Member

Can we just add a NEWS item and close this issue? If so, I'll address this in #4518.

@thomasp85
Copy link
Member

Fine by me. There's already a bullet about the invisible return value of ggsave so maybe just add it there ?

yutannihilation added a commit to yutannihilation/ggplot2 that referenced this issue Jun 17, 2021
@yutannihilation
Copy link
Member

Thanks, added. Does this sound good?

Note that, as a side effect, an unofficial hack <ggplot object> + ggsave() no longer works (#4513).

@ecorreig
Copy link
Author

I still think that the hack adheres more to the adding/piping philosophy than having a ggsave floating around somewhere after the plot, but you can close it, yes, thanks.

@thomasp85
Copy link
Member

I think that sounds fine, yeah

@yutannihilation
Copy link
Member

@ecorreig
I'm sorry, but I'd say it's not the part of "the adding philosophy" as none of the other output functions (i.e. print() and plot()) is allowed to add. For piping, yeah, we all wish ggplot2 had been born after magrittr...

@thomasp85
Thanks, could you just approve #4518?

yutannihilation added a commit that referenced this issue Jun 17, 2021
* Increment version number

* Revert R-CMD-check.yaml

* Add a NEWS item for #4513
@acircleda
Copy link

I used + ggsave() not as a hack but because that is how I learned from others to save ggplot graphics. I think a lot of people do this to pipe in/replace the plot= argument. Any way to make this a feature instead of calling it a hack? I feel like this will break a lot of people's scripts, including mine.

@thomasp85
Copy link
Member

@acircleda when we say that it is a hack we do not necessarily imply any ill-intend on the user, just that it is an accidental functionality that was never promised to work. The fact that some have used it does not really change our stance on it.

adding ggsave did not stop working because we decided to remove the functionality, but because internal wanted changes resulted in the hack stopping to work - such is the nature of hacks.

I can certainly emphasise with your situation but I hope you can understand that development would be completely locked down if we were to take all unintended uses of our code into consideration when fixing bugs etc

@DSLituiev
Copy link

DSLituiev commented Aug 19, 2021

This is indeed very anti-intuitive. Implicit arguments (i.e. taking last plot) are also a prone to problems when working interactively.

Not sure if developers follow how most users believe the code should behave, but if so,
ggplot() %>% ggsave("filename.png") would be much cleaner and intuitive than ggplot() %>% ggsave(file="filename.png", .)

cmkobel added a commit to cmkobel/Roary that referenced this issue Mar 3, 2022
ggplot2 from version 3.3.4 onwards does not support adding ggsave as an object to a plot.
This should be a fix. 
Needs to be tested before a pull request can be sent.

https://ggplot2.tidyverse.org/news/index.html and tidyverse/ggplot2#4513
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 a pull request may close this issue.

6 participants