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.

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

Meta-analysis with MMUPHin and Multi-omics Integration with Cooperative Learning #394

Merged
merged 22 commits into from
Apr 22, 2024

Conversation

YihanLiu4023
Copy link
Contributor

New PR for 2 new chapters: meta-analysis with MMUPHin package and multi-omics integration with cooperative learning.

Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

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

Hello!

First of all thank you for this PR. Looks very nice!! Sorry for taking this long to respond. Unfortunately, I haven't had time to review this in detail. However with quick check, I noticed some things that you might want to consider. I will get back to you when I have checked this more thoroughly.

-Tuomas

inst/pages/IntegratedLearner.qmd Outdated Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Outdated Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Outdated Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Outdated Show resolved Hide resolved
Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

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

  1. All the packages that are used, should be in DESCRIPTION file
  2. Some chunk options were incorrect

I think with these changes this is good to go

inst/pages/IntegratedLearner.qmd Outdated Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Outdated Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Show resolved Hide resolved
inst/pages/MSEA.qmd Outdated Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Outdated Show resolved Hide resolved
inst/pages/MMUPHin_meta_analysis.qmd Outdated Show resolved Hide resolved
inst/pages/IntegratedLearner.qmd Outdated Show resolved Hide resolved
inst/pages/MMUPHin_meta_analysis.qmd Outdated Show resolved Hide resolved
@antagomir
Copy link
Member

Please resolve the review discussions when they have been handled. This way we can keep track on possible issues here that are still pending. When no discussion points are open it is a good sign that this is getting ready.

@antagomir
Copy link
Member

Only very small final changes needed, it seems to me.

@YihanLiu4023 - let us know if you need any support from our side in finalizing these. This is a good opportunity to get in sync with the general development guidelines.

@antagomir
Copy link
Member

Hi! I see see that you're working on this @YihanLiu4023 - just confirm when ready, so we know to proceed checking.

@YihanLiu4023
Copy link
Contributor Author

Hi! I see see that you're working on this @YihanLiu4023 - just confirm when ready, so we know to proceed checking.

Sure! Thank you for the reminder, I have already uploaded all the updates. Please feel free to proceed the checking.

Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

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

I think this is now good to go. Some changes related to build still needed but I can do them.

These chapters are very nice! Thanks!

@TuomasBorman TuomasBorman merged commit d3264df into microbiome:master Apr 22, 2024
inst/pages/MMUPHin_meta_analysis.qmd Show resolved Hide resolved
inst/pages/MMUPHin_meta_analysis.qmd Show resolved Hide resolved
inst/pages/MMUPHin_meta_analysis.qmd Show resolved Hide resolved

library(dplyr)

se_relative <- sampleMetadata |>
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that this is DataFrame (metadata) rather than SummarizedExperiment object (abbreviation "se").

If so, I suggest to rename "se_relative" and "se_pathway" in order to avoid confusing them with SE objects?

Choose a reason for hiding this comment

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

Good point. Thanks for the comment!


```{r load_curatedMetagenomicData}
library(curatedMetagenomicData)

Copy link
Member

Choose a reason for hiding this comment

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

remove the empty line 66?

Comment on lines +219 to +242
##############
# Before
R2_before <- round(fit_adonis_before$aov.tab[1, 5]*100, 1)
pcoa_before <- cmdscale(D_before, eig = TRUE)
ord_before <- as.data.frame(pcoa_before$points)
percent_var_before <- round(pcoa_before$eig / sum(pcoa_before$eig) * 100, 1)[1:2]
before_labels <- c(paste('Axis 1 (', percent_var_before[1], '%)', sep = ''),
paste('Axis 2 (', percent_var_before[2], '%)', sep = ''))

before_phrase <- paste('Before (PERMANOVA R2 = ', R2_before, '%)', sep = '')
colnames(ord_before) <- c('PC1', 'PC2')
ord_before$Study <- data_meta$study_name

# After
R2_after <- round(fit_adonis_after$aov.tab[1, 5]*100, 1)
pcoa_after <- cmdscale(D_after, eig = TRUE)
ord_after <- as.data.frame(pcoa_after$points)
percent_var_after <- round(pcoa_after$eig / sum(pcoa_after$eig) * 100, 1)[1:2]
after_labels <- c(paste('Axis 1 (', percent_var_after[1], '%)', sep = ''),
paste('Axis 2 (', percent_var_after[2], '%)', sep = ''))

after_phrase <- paste('After (PERMANOVA R2 = ', R2_after, '%)', sep = '')
colnames(ord_after) <- c('PC1', 'PC2')
ord_after$Study <- data_meta$study_name
Copy link
Member

Choose a reason for hiding this comment

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

same, mia/SE tools would be rather used (section 7 in OMA has examples)

Comment on lines +244 to +262
# Ordination Plot
p_before <- ggplot(ord_before, aes(x = PC1, y = PC2, color = Study)) +
geom_point(size = 4) +
theme_bw() +
xlab(before_labels[1]) +
ylab(before_labels[2]) +
ggtitle(before_phrase) +
theme(plot.title = element_text(hjust = 0.5)) +
theme(legend.position ="none")

p_after <- ggplot(ord_after, aes(x = PC1, y = PC2, color = Study)) +
geom_point(size = 4) +
theme_bw() +
xlab(after_labels[1]) +
ylab(after_labels[2]) +
ggtitle(after_phrase) +
theme(plot.title = element_text(hjust = 0.5)) +
theme(legend.position ="none")

Copy link
Member

Choose a reason for hiding this comment

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

We have dedicated methods for ordination plots in mia / miaViz

Comment on lines +331 to +332
```{r prepare-input-data-pathway-abundance}

Copy link
Member

Choose a reason for hiding this comment

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

Here also, could we prepare data in SE format, then use tools dedicated to SE.

Comment on lines +381 to +386
D_before <- vegdist(t(data_abd))
D_after <- vegdist(t(data_abd_adj))

set.seed(1)
fit_adonis_before <- adonis(D_before ~ study_name, data = data_meta)
fit_adonis_after <- adonis(D_after ~ study_name, data = data_meta)
Copy link
Member

Choose a reason for hiding this comment

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

See above.

Comment on lines +403 to +418
# Before
R2_before <- round(fit_adonis_before$aov.tab[1, 5]*100, 1)
pcoa_before <- cmdscale(D_before, eig = TRUE)
ord_before <- as.data.frame(pcoa_before$points)
percent_var_before <- round(pcoa_before$eig / sum(pcoa_before$eig) * 100, 1)[1:2]
before_labels <- c(paste('Axis 1 (', percent_var_before[1], '%)', sep = ''),
paste('Axis 2 (', percent_var_before[2], '%)', sep = ''))

before_phrase <- paste('Before (PERMANOVA R2 = ', R2_before, '%)', sep = '')
colnames(ord_before) <- c('PC1', 'PC2')
ord_before$Study <- data_meta$study_name

# After
R2_after <- round(fit_adonis_after$aov.tab[1, 5]*100, 1)
pcoa_after <- cmdscale(D_after, eig = TRUE)
ord_after <- as.data.frame(pcoa_after$points)
Copy link
Member

Choose a reason for hiding this comment

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

same

@antagomir
Copy link
Member

Ok thanks. Now with the more detailed look I noticed one major issue, which is that the scripts are primarily using base R & vegan, rather than the TreeSummarizedExperiment framework. This is somewhat problematic since the entire OMA gitbook is built on the idea of supporting the standardization of methods and shift towards the TreeSummarizedExperiment ecosystem of tools.

Now I think we have two options:

  1. Update the current PR to use SE framework, before merging

  2. Merge now, and then update afterwards, ideally by the original contributor @YihanLiu4023 and/or with support from the broader contributor base. This latter option has the benefit that we can proceed with other things that are pending on merging this PR but this also has a risk that these necessary updates towards SE ecosystem will be delayed.

@YihanLiu4023 @TuomasBorman your comments will be appreciated

@antagomir
Copy link
Member

Ok I can see that this was merged already so I guess we are going with the option 2.

@YihanLiu4023 would you be able to make some of the suggested updates (above pending comments) in a new PR?

@antagomir
Copy link
Member

If the idea is to update this to IntegratedLearning and that will support SE/MAE, then we can deal with these in the next round?

@TuomasBorman
Copy link
Contributor

I agree with your points @antagomir . Sorry for merging it before you had time to give your review.

The material in chapters are very good addition but now they are not using TreeSE ecosystem which is a big lack. However, I believe it was better to merge now and improve later. I understood that these chapters will be updated later. (Also great chance to integrate TreeSE to methods that are not supporting it yet)

@antagomir
Copy link
Member

@YihanLiu4023 could you summarize the planned updates and their planned schedule?

@antagomir
Copy link
Member

I just noticed that these new chapters are not rendered as part of the OMA gitbook https://microbiome.github.io/OMA/

I think the reason is that the qmd file names have not been listed here:
inst/assets/_book.yml

@YihanLiu4023 did you check that you can render and view the book locally, before opening the PR?

@TuomasBorman
Copy link
Contributor

This issue is already handled with PR https://github.com/microbiome/OMA/pull/433/files

@himelmallick
Copy link

I agree that going with Option 2 and updating later. We will make sure to use the SE/TSE framework in the IntegratedLearner R package which can be added back to the chapters instead of the base R functions and the same thing applies to the other packages. Thanks all for your comments. @YihanLiu4023 will work on the immediately addressable comments from above.

@antagomir
Copy link
Member

great to hear, we needed to merge but let's continue and take it forward together; don't hesitate to ask if anything is unclear as the ecosystem and documentation are shaping up and we can improve where necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants