fixed byrow=FALSE reorder not only plots but also labels#203
fixed byrow=FALSE reorder not only plots but also labels#203clauswilke merged 3 commits intowilkelab:masterfrom
byrow=FALSE reorder not only plots but also labels#203Conversation
Add the function which reoder the labels when byrow=F
|
Please consider merging - this seems like a simple and straightforward bugfix. I just ran into this bug in the wild and found it quite surprising. |
| # if the user wants to layout the plots by column, we use the calculated rows to reorder plots and labels | ||
| if (!isTRUE(byrow)) | ||
| plots <- plots[c(t(matrix(c(1:num_plots, rep(NA, (rows * cols) - num_plots)), nrow = rows, byrow = FALSE)))] | ||
| labels <- labels[c(t(matrix(c(1:num_plots, rep(NA, (rows * cols) - num_plots)), nrow = rows, byrow = FALSE)))] |
There was a problem hiding this comment.
Shouldn't this code block be enclosed in curly braces? Otherwise line 183 gets executed regardless of whether the condition is true or not, no? Either way, please add curly braces for clarity.
|
Apologies, yes, this is fine in principle. Is it possible to add a simple unit test that tests for the order of the labels in both arrangements? Here is some starting code for the unit tests: p_list <- map(1:3, \(x) ggplot())
g <- plot_grid(plotlist = p_list, ncol = 2, labels = 1:3, byrow = FALSE)
layer_data(g, 4)Just extract all the labels by making the relevant |
|
@Doubt-0KB I don't know what you did with your last commit but it's not a clean diff. You probably messed up newlines. Please revert. |
|
Sorry, I'm not familiar with the operation and made a mistake. I will revert and commit again. |
|
I apologize for the inconvenience caused by my incorrect commit operation. |
tests/testthat/test_plot_grid.R
Outdated
|
|
||
| p_list <- lapply(1:3, \(x) ggplot()) | ||
|
|
||
| expect_doppelganger("byrow is TRUE", |
There was a problem hiding this comment.
No visual tests please. (I meant to say this in my original comment but now reading it again I did not explicitly do so.) Visual tests cause constant headaches, and they get disabled all the time because the false positive rate is so high.
Instead, you can extract the location of the labels using the code I posted (e.g., layer_data(g, 4)) and check that the labels are placed in the correct location.
|
I remove the visual tests, and add the tests using the code you posted. |
|
Thanks! |
3 participants
This PR fixes an issue where the argument
byrow=FALSEonly reorders the plots, but not the labels. Closes #199.Here is a demo of fixed function.