xuyiqing / gsynth

Generalized Synthetic Control Method
Other
131 stars 40 forks source link

Use return() to return plots from plot.gsynth() instead of suppressWarnings(print(p)) #58

Open fschaffner opened 3 years ago

fschaffner commented 3 years ago

Hi, thanks a lot for creating and maintaining this great package!

I noticed that in the plot.gsynth() function you are using suppressWarnings(print(p)) to return plots instead of return(p). This poses the following problems:

  1. A user can't assign the plot to a new object without actually having the plot show up in the RStudio Viewer. For example, one would expect that gsynth_plot <- plot(out) assigns the ggplot to the object gsynth_plot, but that this plot is not displayed in the RStudio Viewer. Only once the user calls the assigned object gsynth_plot the plot should appear in the Viewer.

  2. If the user wants to supply their own ggplot theme, e.g. plot(out) + theme_dark(), there will be two plots displayed in the Viewer, one with the default theme, and one with the theme that the user supplied. Moreover, if plot(out) + theme_dark() is included in Rmarkdown, the plot will appear twice in the knitted output, once with the default theme, and once with the dark theme.

This can easily be solved, by simply replacing all instances of suppressWarnings(print(p))with return(p). I have implemented this in my fork over at https://github.com/fschaffner/gsynth and it works well. Do you want me to send a pull request?

xuyiqing commented 3 years ago

Hi Florian,

We'll look into this. For (2), I think you can still access the plot via $graph in a gsynth object. Is that right, Licheng?

I think we add this to address an issue in RMarkdown.

Best, Yiqing

On Fri, Apr 9, 2021 at 4:01 PM Florian Schaffner @.***> wrote:

Hi, thanks a lot for creating and maintaining this great package!

I noticed that in the plot.gsynth() function you are using suppressWarnings(print(p)) to return plots instead of return(p). This poses the following problems:

1.

A user can't assign the plot to a new object without actually having the plot show up in the RStudio Viewer. For example, one would expect that gsynth_plot <- plot(out) assigns the ggplot to the object gsynth_plot, but that this plot is not displayed in the RStudio Viewer. Only once the user calls the assigned object gsynth_plot the plot should appear in the Viewer. 2.

If the user wants to supply their own ggplot theme, e.g. plot(out) + theme_dark(), there will be two plots displayed in the Viewer, one with the default theme, and one with the theme that the user supplied. Moreover, if plot(out) + theme_dark() is included in Rmarkdown, the plot will appear twice in the knitted output, once with the default theme, and once with the dark theme.

This can easily be solved, by simply replacing all instances of suppressWarnings(print(p))with return(p). I have implemented this in my fork over at https://github.com/fschaffner/gsynth and it works well. Do you want me to send a pull request?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/xuyiqing/gsynth/issues/58, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB2PKGBL2XQPB5GLJLIPNGLTH6BL7ANCNFSM42VZ2RJA .