Issue #174: TreeSE support non-taxonomic ranks #201
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 = union(mia::taxonomyRanks(tse), rownames(SummarizedExperiment::rowData(tse))) | ||
| txt = sprintf(paste0("`tax_level` is not specified \n", | ||
| "No agglomeration will be performed", | ||
| "\n", | ||
|
|
@@ -41,7 +41,15 @@ tse_construct = function(data, assay_name, tax_level, phyloseq) { | |
| tax_level = "ASV" | ||
| tse_alt = tse | ||
| } else { | ||
| # 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 | ||
Daenarys8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| SingleCellExperiment::altExp(tse, tax_level) = tse_alt | ||
| } else if (!is.null(phyloseq)) { | ||
|
|
@@ -57,7 +65,7 @@ tse_construct = function(data, assay_name, tax_level, phyloseq) { | |
| } | ||
|
|
||
| if (is.null(tax_level)) { | ||
| tax_levels = union(mia::taxonomyRanks(tse), rownames(SummarizedExperiment::rowData(tse))) | ||
Daenarys8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| txt = sprintf(paste0("`tax_level` is not speficified \n", | ||
|
There was a problem hiding this comment. @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 There was a problem hiding this comment. The commit is ready. |
||
| "No agglomeration will be performed", | ||
| "\n", | ||
|
|
@@ -68,7 +76,15 @@ tse_construct = function(data, assay_name, tax_level, phyloseq) { | |
| tax_level = "ASV" | ||
| tse_alt = tse | ||
| } else { | ||
| # 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 | ||
|
There was a problem hiding this comment. Should we export mia::.merge_features? There was a problem hiding this comment. No but I think you can it with triple semicolon. Check with TB There was a problem hiding this comment. notably mia:::.merge_features is available from mia 1.9.9 There was a problem hiding this comment. As pointed out by @Daenarys8 that the > mia:::.merge_features
Error: object '.merge_features' not foundI'm wondering the best approach to import this function. One option is to consider copying the function directly from this source into the Thank you, |
||
| } | ||
| SingleCellExperiment::altExp(tse, tax_level) = tse_alt | ||
| } else { | ||
|
|
||
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.
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 viamia::mergeFeaturesByRank(). The internal functionmia::.merge_featureswas 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.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 that
mia::mergeFeatures()automatically checks whether the field is in taxonomy ranks or not and then calls the internal function based on that.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.
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.
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.
The
.merge_featuresis internal to mia and should not be imported. But I think it can also be called if necessary, like withmia:::.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?