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.

Sign up for GitHub

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

Jump to bottom

Issue 161: by column with plot_grid #163

Merged
merged 6 commits into from
Aug 19, 2020
Merged

Issue 161: by column with plot_grid #163

merged 6 commits into from
Aug 19, 2020

Conversation

BrianLang
Copy link
Contributor

closes #161

  • merged to master
  • rebuilt documentation
  • added visual test
  • validated visual test

@BrianLang
Issue 161: by column with plot_grid
861c1b3 
- Added argument bycol to function call.
- Calculate plots ordering if bycol is TRUE.
- Moved grobs calculation to after plots ordering.
@BrianLang
- Changed argument name to byrow.
bede962 
- Moved argument name position in function call.
- Use FALSE instead of F.
- Updated function documentation.
- Added an example under `\donttest`
- Updated NEWS.md
@BrianLang
Fix code styling plot_grid
01a4952
@BrianLang
closes wilkelab#161
d73f925 
- merged to master
- rebuilt documentation
- added visual test
- validated visual test
@BrianLang
Copy link
Contributor Author

Somehow I'm still figuring out how to do all of this. Was there a way to do what you asked (merge to master, fix things, commit, and push) without opening a new pull request?

@clauswilke
Copy link
Contributor

Yes, you can just keep pushing commits and github automatically sees them and updates the PR. No problem though, we can work with this one.

@clauswilke
Copy link
Contributor

Sorry, one more issue. Can you try your plot reordering code for a case where num_plots is less than rows * cols? I think your matrix() call will create a warning, and you should be able to fix this by padding 1:num_plots with NAs. Maybe this will work: c(1:num_plots, rep(NA, rows * cols - num_plots))

clauswilke
clauswilke reviewed Aug 19, 2020
View reviewed changes
tests/testthat/test_plot_grid.R Outdated
@@ -27,6 +27,10 @@ test_that("basic plot arranging works", {
expect_doppelganger("scaling plots",
plot_grid(p1, p2, p3, labels = "AUTO", scale = c(.9, .7, .5), nrow = 1) + theme_map() # add theme_map() for plot title
)

expect_doppelganger("colwise arranging",
plot_grid(p1, NULL, p2, NULL, p3, NULL, byrow = FALSE) + theme_map()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd delete the last NULL argument to test the case that the number of plots is less than rows * cols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, yes, of course this could break things. I'll check and fix it: adding a visual test that assesses the case where the number of plots is less than rows * cols

@BrianLang
matrix issues when num_plots < row * col
f764845 
- Matrix normally pads via recycling.
- Pad matrix with NAs instead.
- Update visual test to be more specific to when `num_plots < row * col`.
@BrianLang
Copy link
Contributor Author

You were correct that having num_plots < cols * rows produces warnings and issues, specifically that matrix recycles inputs. So I've added your suggestion of padding with c(1:num_plots, rep(NA, rows * cols - num_plots)) and it looks correct now.

It also is coincidental the case that the previous visual test gave a false "valid" for dropping the last NULL, so I've updated it to something that wouldn't pass given the previous implementation.

expect_doppelganger("colwise arranging",
   plot_grid(p1, NULL, p2, p3, NULL, byrow = FALSE) + theme_map() # add theme_map() for plot title
 )

@clauswilke
Copy link
Contributor

Thanks!

@clauswilke clauswilke merged commit 0c4db21 into wilkelab:master Aug 19, 2020
@BrianLang
Copy link
Contributor Author

Woooo! First merged pull request! Thanks for your help :-)

@Dicky3333
Copy link

Thanks Claus and Brian for working on this request. I updated the cowplot package with install.package("cowplot"). But the plot_grid() method doesn't seem to have the new 'byrow' parameter. I'd appreciate if you could tell me how to get the updated package.

@clauswilke
Copy link
Contributor

You'll have to install the development version.

remotes::install_github("wilkelab/cowplot")

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

@clauswilke clauswilke clauswilke left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Plotting ggplots 'by column' with plot_grid()
3 participants
@BrianLang @clauswilke @Dicky3333