Conversation
antagomir
left a comment
antagomir
left a comment
There was a problem hiding this comment.
Awesome! Just some suggestions.
|
Now the vignettes use DAA results from Maaslin3. While testing, I also added plotForest visualisation to the differential abundance chapter of OMA, for all the different methods. I could open a PR there. |
TuomasBorman
left a comment
TuomasBorman
left a comment
There was a problem hiding this comment.
Some comments. Looks good!
| # Generate forest plot components | ||
| plots <- .plot_forest( | ||
| x = x, | ||
| effect.var = effect.var, | ||
| ci.lower.var = ci.lower.var, | ||
| ci.upper.var = ci.upper.var, | ||
| err.var = err.var, | ||
| pval.var = pval.var, | ||
| id.var = id.var, | ||
| label.by = label.by, | ||
| order.by = order.by, | ||
| facet.by = facet.by, | ||
| color.by = color.by, | ||
| conf.level = conf.level | ||
| ) |
There was a problem hiding this comment.
This function could be splitted. The recommendation of function length is under 50 lines. For instance it can be splitted:
- Check input
- Do all the data wrangling, the data should be directly ready for plotting
- Do the plotting
There was a problem hiding this comment.
It's a bit tricky because its parts are quite inter-dependent. I'll check if I can come up with a clever way to split it.
There was a problem hiding this comment.
I did the best I could to break it into parts. How does it look now?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #220 +/- ##
==========================================
+ Coverage 57.71% 61.39% +3.68%
==========================================
Files 21 22 +1
Lines 3493 3865 +372
==========================================
+ Hits 2016 2373 +357
- Misses 1477 1492 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TuomasBorman
left a comment
TuomasBorman
left a comment
There was a problem hiding this comment.
Sorry for the delay. Here are couple final comments; most of them are thoughts and can be ignored if justified
| "specified with 'pval.var'.", call. = FALSE) | ||
| } | ||
| # Check aesthetics | ||
| aes.list <- list(order.by = order.by, facet.by = facet.by, color.by = color.by) |
There was a problem hiding this comment.
Elswehere normal variables are with underscore
aes_list.
I would change this
| # Check kwargs | ||
| kwargs <- list(...) | ||
| if( !show.tree && length(kwargs) != 0 ){ | ||
| warning("Arguments in '...' are ignored when 'show.tree' is FALSE.", | ||
| call. = FALSE) | ||
| } |
There was a problem hiding this comment.
I might also drop this check, because this is already told here
@param ... additional parameters passed to \code{\link{plotRowTree}}.
Also I feel that this is easily forgotten, if support for optional arguments is added in the future (for instance tweaking line sizes etc)
| #' colour.by = "association" | ||
| #' ) | ||
| #' | ||
| #' @author Giulio Benedetti |
There was a problem hiding this comment.
This could be removed (can you also remove plotAbundanceDensity.R line 96). Functionas are collectively maintained and GH shows exact contributions already
| tree.FUN <- switch(by, rowTree, colTree) | ||
| treename.FUN <- switch(by, rowTreeNames, colTreeNames) | ||
| treeplot.FUN <- switch(by, plotRowTree, plotColTree) | ||
| # Extract side information from SE |
There was a problem hiding this comment.
As a first step, you could run
.get_object_and_trimmed_tree()
TreeSE can include multiple trees. It seems that this is not fully taken into account yet. E.g., it can be that the rows in TreeSE do not match with nodes in plotted tree (because the rows are included in the second tree)
| } | ||
|
|
||
| #' @importFrom patchwork wrap_plots | ||
| .combine_forest_components <- function(x, effect.var, ci.lower.var, ci.upper.var, |
There was a problem hiding this comment.
Run BiocCheck::BiocCheck(). I guess this line is too wide
| p <- plotForest(df2, id.var = "ID") | ||
| expect_s3_class(p, "ggplot") |
There was a problem hiding this comment.
Is it possible to add test that checks that the values are correct?
3 participants
Hi!
The plotForest viz is ready to be shared! (check vignettes for usage)
It features methods for both (Tree)SE and dataframes containing statistical results. All relevant variables (effects, CI upper and lower boundaries, pvals, id cols) are encoded by arguments for convenience and flexibility.
plotForest can be applied to both rowData and colData, with the option to add tree hierarchy and labels on the side, and also comparing between two groups (paired forest plot).
While minor aesthetic add-ons could be considered for future PRs, I think the main functionality is already there and is quite broad.
Happy to hear your comments,
Giulio