search

microbiome / mia

Microbiome analysis
https://microbiome.github.io/mia/
Artistic License 2.0
47 stars 28 forks source link

splitOn adds rowTree instead of manipulating existing one #494

Closed thpralas closed 5 months ago

thpralas commented 7 months ago

Describe the bug After splitting the data, if udpate_rowTree is TRUE, splitOn() adds a new rowTree to all TreeSummarizedExperiment objects in the returned list instead of manipulating the existing rowTree.

if( class(x) == "SimpleList" ){
    x <- SimpleList(lapply(x, addTaxonomyTree))
}
TuomasBorman commented 7 months ago

Thanks!

Can you make a quick try if you can fix the bug?

See https://github.com/microbiome/mia/pull/487/files#diff-86741c1d199054c30214beca158175167a1913b4e6a77e89f89b625713d04c5bR296-R298

I believe you can just

  1. Create a new branch
  2. replace addTaxonomyTree with .agglomerate_trees.
  3. Add some tests to unit tests. Check that these elements in returned list have length(tree$tip.labels) == nrow(tse) when udpate_rowTree = TRUE
  4. Bump the version. Add description of this fix to NEWS file.

Ask if unclear, do not spend too much time if it seems that the problem is harder to fix than this

thpralas commented 7 months ago

I just made a commit referencing this issue.

I replaced addTaxonomyTree with .agglomerate_trees and added tests. The tests all passed but for each TreeSummarizedExperiment in the returned list, I received the following warning from the agglomerate_tree function : 'keep.nodes' does specify all the tips from 'tree'. The tree is not agglomerated..

When I ran the test without replacing addTaxonomyTree, some tests failed so I think the new function works but I don't know if the tests I added are relevant.