-
Notifications
You must be signed in to change notification settings - Fork 12
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
Initialise plotMediation method #165
base: devel
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me, I just added some comments.
if(p_mat[i, j] < 0.001) { | ||
grid.text("***", k, y) | ||
} else if(p_mat[i, j] < 0.01) { | ||
grid.text("**", k, y) | ||
} else if(p_mat[i, j] < 0.05) { | ||
grid.text("*", k, y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could / should these thresholds be something that the user could define? and these ones here could be the default values? It could be for instance one argument, like signif.threshold = c(0.001, 0.01, 0.05) or if it is just signif.threshold = c(0.01, 0.05) with two values, then only two levels of stars etc?
p <- Heatmap(coef_mat, name = "Effect", | ||
cluster_rows = FALSE, cluster_columns = FALSE, | ||
row_names_side = "left", column_names_side = "top", | ||
column_names_rot = 0, rect_gp = gpar(col = "white", lwd = 2), | ||
cell_fun = cell_fun) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression is that it would be good if users can pass these arguments, so these would come via the "..." mechanism in the function call? And the manpage could mention that the ComplexHeatmap::Heatmap arguments are supported. Or at least some critical ones.
imo ComplexHeatmap could be added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not add ComplexHeatmap
as dependency.
- This can be achieved with
ggplot2
- Even though
ComplexHeatmap
is easier to plot, it is harder to modify afterwards - It causes lack of standardization (I think all the plots in
miaViz
should be done withggplot
as it is the gold standard, if possible) - It makes it confusing if the package outputs plots in different formats
#' @rdname plotMediation | ||
#' @export | ||
setGeneric("plotMediation", function(x, ...) standardGeneric("plotMediation")) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add generics to AllGenerics.R
This is a script that does the same in
|
Also, I found that this "metadata" is rather mess. Does it come directly from
|
If ComplexHeatmap is not added as a dependency, should we still consider showing an example with that as well, in OMA for instance? |
Yes, and we have currently and example on that, for instance: https://microbiome.github.io/OMA/docs/devel/pages/correlation.html#association-between-taxa-and-sample-metadata |
Thanks for the comments! I agree it is better to use standard ggplot2. The metadata is a list of model outputs for each mediator used, as someone might want to have access to the original mediation model output. I can check if that can be organised in a table. |
In microbiome/mia#678 the messy metadata is polished. If that looks good, I will then proceed with plotMediation. |
Hi!
This PR introduces the plotMediation method meant to visualise results from mia functions getMediation and addMediation.
It is based on ComplexHeatmap but if you want to keep miaViz depend only on ggplot I can change to ggplot.
It would be nice to have a barplot option but it complicates the method because data like confidence intervals is not stored directly in the main output.
Feel free to give your thoughts!