Issue #174: TreeSE support non-taxonomic ranks #201

Daenarys8 · 2023-08-24T08:09:22Z

#174
@antagomir with some suggestions, I can update the docs as well.

Signed-off-by: Daenarys8 <[email protected]>

antagomir

Let's more directly utilize the already existing and well tested functionality from mia instead of reimplementing checks here, if it can be avoided.

antagomir · 2023-08-24T08:23:17Z

R/ancombc_prep.R

@@ -30,7 +30,7 @@ tse_construct = function(data, assay_name, tax_level, phyloseq) {

        # Check if agglomeration should be performed
        if (is.null(tax_level)) {
-            tax_levels = mia::taxonomyRanks(tse)
+            tax_levels = union(mia::taxonomyRanks(tse), rownames(SummarizedExperiment::rowData(tse)))


Hmm I think this is mistaken. The rownames refer to individual taxa (rownames of rowData), not the high-level categories of those taxa (colnames of rowData). Moreover, we already have functionality to agglomerate TreeSE objects based on any rowData field, through the function mia::mergeFeatures() and for taxonomic ranks specifically via mia::mergeFeaturesByRank(). The internal function mia::.merge_features was intended to check whether the given field is in taxonomyRanks or not, and then choose the merging function based on that. That was the entire motivation for the recent PR work in mia. Therefore there is no need to take union of different field names here but instead we should use the background work that was already laid out in mia to automatically handle these things, if possible.

I think that mia::mergeFeatures() automatically checks whether the field is in taxonomy ranks or not and then calls the internal function based on that.

ACK. mia::mergeFeaturesByRank was not exported. There is a PR for that now. Similarly mia::.merge_features is not exported. mia::mergeFeatures() deprecated mia::mergeRows.
The union to give tax_levels is to display in the message the possible tax_level params when it is NULL.

The .merge_features is internal to mia and should not be imported. But I think it can also be called if necessary, like with mia:::.merge_features (/w three double colons). To check.

Let's first merge the relevant PRs in mia and solve this based on those.

We can discuss in Slack or in mia issues whether we should have exported function that combines these two, or do we want to always separate them and create different combinations for different situations and packages like here now?

R/ancombc_prep.R

antagomir · 2023-08-24T08:28:49Z

R/ancombc_prep.R

+            # Check if tax_level parameter belongs to taxonomyRanks
+            if (is.character(tax_level) && length(tax_level) == 1 && tax_level %in% mia::taxonomyRanks(tse)) {
+                #Merge using agglomerateByRank
+                tse_alt <- mia::agglomerateByRank(tse, tax_level)
+            } else {
+                # Merge using mergeRows
+                tse_alt <- mia::mergeRows(tse, tax_level)
+            }
+            tse_alt


Should we export mia::.merge_features?

No but I think you can it with triple semicolon. Check with TB

notably mia:::.merge_features is available from mia 1.9.9

As pointed out by @Daenarys8 that the mia:::.merge_features function becomes available starting from mia version 1.9.9, while the currently released version of mia on Bioconductor remains at 1.8.0. Consequently, the ".merge_features" function is currently not accessible.

> mia:::.merge_features Error: object '.merge_features' not found

I'm wondering the best approach to import this function. One option is to consider copying the function directly from this source into the ANCOMBC utils functions. I will update the package accordingly in the future once this function becomes available in the release version of mia. Any ideas? @Daenarys8 @antagomir

Thank you,
Huang

antagomir · 2023-08-24T08:30:25Z

Updating the docs would be very useful. Which docs exactly? manpages, vignettes, in ANCOMBC?

Signed-off-by: Daenarys8 <[email protected]>

antagomir

Also check that the roxygen documentation is up-to-date, and the roxygen examples field and vignette include an example that is up-to-date.

antagomir · 2023-09-03T07:37:10Z

R/ancombc_prep.R

@@ -30,7 +30,7 @@ tse_construct = function(data, assay_name, tax_level, phyloseq) {

        # Check if agglomeration should be performed
        if (is.null(tax_level)) {
-            tax_levels = mia::taxonomyRanks(tse)
+            tax_levels = union(mia::taxonomyRanks(tse), colnames(SummarizedExperiment::rowData(tse)))


Is this needed? the rowData may contain also other features than taxonomic ranks, do we need to define them as tax_levels? What is the intended purpose of this line?

Not necessarily

antagomir · 2023-09-03T07:37:35Z

R/ancombc_prep.R

@@ -57,7 +57,7 @@ tse_construct = function(data, assay_name, tax_level, phyloseq) {
        }

        if (is.null(tax_level)) {
-            tax_levels = mia::taxonomyRanks(tse)
+            tax_levels = union(mia::taxonomyRanks(tse), colnames(SummarizedExperiment::rowData(tse)))
            txt = sprintf(paste0("`tax_level` is not speficified \n",


@Daenarys8 @antagomir Thank you both for addressing the taxonomy rank issue, I really appreciate it. I seem to have lost track of the conversation. Could you please confirm whether the commit is now ready for merging? I tend to publish a new version of ANCOMBC in a week. Thanks!

The commit is ready.

antagomir · 2023-09-11T22:41:04Z

In fact I was wondering the same if @Daenarys8 can comment, thanks!

Signed-off-by: Daenarys8 <[email protected]>

antagomir · 2023-09-15T09:41:12Z

It is ready as @Daenarys8 says but I would still have some requests if time allows. Otherwise, we can add these in the next round of updates.

The Bioc framework has switched from argument assay_name to assay.type and it would be important to make this update also in ANCOMBC for consistency
The tax_level is used in phyloseq ( I think) but in TreeSE world we use rank. Could we add an alias for the argument, so that one could also use "rank" as in TreeSE?
It seems possible to use the parameter passing (...) in function calls more than now, this would help to avoid specifying the arguments many times (see mia for examples). This change might take a bit more thinking and is easier to postpone if things otherwise work as expected.

SOme more testing about interoperability will be useful but also easier after merging this.

FrederickHuangLin · 2023-09-15T19:05:50Z

Thank you, @antagomir, for the heads-up!

I'm planning to publish a new version of ANCOMBC early next week, most likely on Monday or Tuesday. It might be more convenient to incorporate the updates you mentioned in the next iteration.

Once again, I appreciate your contribution!

Best regards,
Huang

Issue 174: TreeSE support non-taxonomic ranks

dabfb13

Signed-off-by: Daenarys8 <[email protected]>

antagomir suggested changes Aug 24, 2023

View reviewed changes

Daenarys8 added 3 commits August 25, 2023 07:52

Up

f598412

Signed-off-by: Daenarys8 <[email protected]>

Up

772dc6d

Signed-off-by: Daenarys8 <[email protected]>

Up

69e0078

Signed-off-by: Daenarys8 <[email protected]>

antagomir suggested changes Sep 3, 2023

View reviewed changes

Up

73b9979

Signed-off-by: Daenarys8 <[email protected]>

FrederickHuangLin merged commit 9e617f3 into FrederickHuangLin:bugfix Sep 16, 2023

antagomir mentioned this pull request Sep 17, 2023

ANCOMBC microbiome/OMA#345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #174: TreeSE support non-taxonomic ranks #201

Issue #174: TreeSE support non-taxonomic ranks #201

Daenarys8 commented Aug 24, 2023

antagomir left a comment

antagomir Aug 24, 2023

antagomir Aug 24, 2023

Daenarys8 Aug 25, 2023

antagomir Aug 25, 2023

antagomir Aug 24, 2023

Daenarys8 Aug 25, 2023

antagomir Aug 25, 2023

Daenarys8 Sep 14, 2023

FrederickHuangLin Sep 16, 2023

antagomir commented Aug 24, 2023

antagomir left a comment

antagomir Sep 3, 2023

Daenarys8 Sep 14, 2023

antagomir Sep 3, 2023

FrederickHuangLin Sep 11, 2023 •

edited

Loading

Daenarys8 Sep 14, 2023

antagomir commented Sep 11, 2023

antagomir commented Sep 15, 2023

FrederickHuangLin commented Sep 15, 2023

Issue #174: TreeSE support non-taxonomic ranks #201

Issue #174: TreeSE support non-taxonomic ranks #201

Conversation

Daenarys8 commented Aug 24, 2023

antagomir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antagomir commented Aug 24, 2023

antagomir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrederickHuangLin Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antagomir commented Sep 11, 2023

antagomir commented Sep 15, 2023

FrederickHuangLin commented Sep 15, 2023

FrederickHuangLin Sep 11, 2023 •

edited

Loading