add class and function to create a plot grid #862
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
IndrajeetPatil
requested review from
msevestre,
PavelBal,
Yuri05 and
pchelle
and removed request for
msevestre
|
Builds are passing. This PR is finished from my end. |
This comment was marked as off-topic.
This comment was marked as off-topic.
| #' | ||
| #' library(ospsuite) | ||
| #' library(ggplot2) | ||
| #' library(tlf) |
There was a problem hiding this comment.
I think library(ggplot2) is not required - tlf loads ggplot automatically
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| plotGridObj$tagSuffix <- ")" | ||
|
|
||
| set.seed(123) | ||
| vdiffr::expect_doppelganger( |
There was a problem hiding this comment.
Roughly speaking, it checks if the generated image matches the existing SVG representing the expected plot.
There was a problem hiding this comment.
@msevestre, @pchelle Btw, vdiffr is what you could be using to test tlf (cf. Open-Systems-Pharmacology/TLF-Library#92), just like in this PR.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| #' @param plotList A list containing `ggplot` objects. | ||
| #' | ||
| #' @return A `PlotGridConfiguration` object. | ||
| initialize = function(plotList = NULL) { |
There was a problem hiding this comment.
@IndrajeetPatil if you can't add plots after the fact, this argument should be required and non null (and probably with at least one element no?)
There was a problem hiding this comment.
I am fine with either of the options:
-
force users to provide at least a plot list when constructor is called (in which case it should not be NULL)
-
allow creating an empty instance and then users can add plots using the public field (in which case this should be NULL)
R/plotGrid.R
Outdated
| plotGrid <- function(plotGridConfiguration) { | ||
| patchwork::wrap_plots( | ||
| PlotGridConfiguration$plotList, | ||
| ncol = PlotGridConfiguration$nColumns, |
There was a problem hiding this comment.
there is a mix of instance plotGridConfiguration and class PlotGridConfiguration going on here. I don't get it
There was a problem hiding this comment.
Yeah, that was unintentional and introduced in my latest commit.
I will fix it tomorrow.
There was a problem hiding this comment.
Fixed this.
Following the coding convention-
Class name: UpperCamelCase (PlotGridConfiguration)
Parameter name: lowerCamelCase (plotGridConfiguration)
|
Don't forget to specify in one of your commit "Fixes #xx" otherwise the issue won't close automatically |
|
We didn't have any issue to link to for this PR. We have a related discussion but AFAIK linking to discussions in a PR doesn't do anything. |
|
I see. Let's create issues for code changes next time. These issues , if they are based on discussions, can be succinct and simply point to an ongoing discussion. |
|
Done: #865 |
createPlotGridConfiguration()ospPlotGridConfigurationplotGrid()Related to #856
Part of #674