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

Fix the "two chunk" requirement for rendering ggMarginalPlots in RMarkdown docs #148

Merged
merged 2 commits into from
Aug 20, 2019

Conversation

crew102
Copy link
Contributor

@crew102 crew102 commented Aug 20, 2019

This PR provides my best attempt at solving #147. It has the issue of the leading blank page that I mentioned in that issue thread, though other than that it pretty much gets the job done. I'll update the README once/if this PR gets the all clear. A few questions that may cross your mind:

  1. Why not use the same solution of converting the ggMarginalPlot into a ggplot, regardless of whether the plotting code is evaluated in an RMarkdown doc? I.e., why not just always use lines https://github.com/daattali/ggExtra/compare/master...crew102:ggmarginals-in-rmarkdown?expand=1#diff-01bed624663254513627969554598880R206-R210?

If we convert to a ggplot object, we end up with a leading blank page when plotting to non-interactive devices. This can be fixed by using ggsave() instead of following the workflow of opening up a graphics device, plotting, then closing the device, but that would require people change all their code that writes ggMarginalPlots to files.

  1. Will the current solution still allow users to use the two chunk solution proposed in the README?

Yes, the two chunks approach will still produce the same results as it always has. This is still the only way to avoid the leading blank page when running a chunk semi-interactively (i.e., when pressing the "run current chunk" button).

  1. Why do you have to convert the ggExtraPlot/gtable object returned by ggMarginal() into a ggplot to address the issues mentioned in Rendering inline in rstudio #89 and https://support.rstudio.com/hc/en-us/community/posts/239529128-Notebooks-grid-newpage?

No idea.

  1. Could the solution of converting the ggMarginalPlot to a ggplot somehow provide the functionality desired in any possibility of not changing class of plots #129?

I don't think so, though I didn't take a very close look. Essentially when you convert the ggMarginalPlot to a ggplot, the entire plot (including the marginal plots) are considered the plot. So, for example, if you tried to change the theme of the plot, it would change the theme of the scatter plot and the marginal plots. It may be possible to pull out the scatter plot and manipulate it on its own, but I don't think that's @IndrajeetPatil was going for in #129.

@daattali
Copy link
Owner

With this solution, should a notebook file containing this chunk

```{r}
p <- ggplot2::ggplot(mtcars, ggplot2::aes(wt, mpg)) + ggplot2::geom_point()
ggExtra::ggMarginal(p)
```

produce the plot?

@crew102
Copy link
Contributor Author

crew102 commented Aug 20, 2019

It shoulllld. Does it?

@daattali
Copy link
Owner

Not for me. Does it work for you? Is there any other way to test this PR?

@crew102
Copy link
Contributor Author

crew102 commented Aug 20, 2019

Yeah, so this is probably a mac v windows thing. Not sure where we go from here. The current two chunk solution works across all platforms and doesn't produce the leading page. The solution in this PR seems to work on osx (at least in my env) and doesn't work on WIndows. We could compare RStudio/ggplot2 versions, but I'm not sure if that would provide more clarity.

I'm willing to say "this one-chunk solution seems to work in some cases" in the readme, and tell users to fallback to the two-chunk alternative. It's kinda a wash for me, though. Thoughts, @daattali and @IndrajeetPatil ?

rmarkdown-ggextra

@daattali
Copy link
Owner

If we merge this PR, it will fix the issue in some situations. Is there also any regressions or other changed behaviour you expect?

@crew102
Copy link
Contributor Author

crew102 commented Aug 20, 2019 via email

@daattali daattali merged commit 9c3bdb5 into daattali:master Aug 20, 2019
@daattali
Copy link
Owner

daattali commented Aug 20, 2019

Thanks for all the hard work - I'm sure the time it took to come up with these few lines is not proportional to the number of lines you added. And thanks for your detailed message too - the answer to # 3 was especially informative :)

@IndrajeetPatil
Copy link
Contributor

Thank you so much for all your work and understanding!!

Do you plan to submit the newer version to CRAN any time soon?

@crew102
Copy link
Contributor Author

crew102 commented Aug 23, 2019

@daattali , what are your thoughts on releasing a new version? I think you have to be the one to submit to CRAN b/c you're the maintainer.

@daattali
Copy link
Owner

I'll work on submitting to CRAN soon, thanks for your patience!

@daattali
Copy link
Owner

Package has been submitted

@crew102 crew102 deleted the ggmarginals-in-rmarkdown branch April 21, 2020 01:39
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.

3 participants