search

etetoolkit / ete

Python package for building, comparing, annotating, manipulating and visualising trees. It provides a comprehensive API and a collection of command line tools, including utilities to work with the NCBI taxonomy tree.
http://etetoolkit.org
GNU General Public License v3.0
768 stars 216 forks source link

Refactor set outgroup function and update tests #713

Closed jhcepas closed 8 months ago

jhcepas commented 9 months ago

@jordibc, I have created a new root_at() function that uses smartview gardening methods but respects (hopefully) previous behaviour of set outgroup.

The main difference between your root_at and set_outgroup was that the absolute tree root instance was never changed in set_outgroup, so there was no need to return the new root node.

I also had to change tree.populate() so it does not initialize the tree root node with default and arbitrary support and dist values, which makes no sense now that we handle the absence of values explicitly.

I updated all the tests accordingly, which are apparently passing now.

btw, it looks weird that set_outgroup() is importing gardening from smarview.rendered. Wouldn't make more sense to have such module close tree.pyx and smarview importing from there?

Please review and let me know!

jordibc commented 9 months ago

@jhcepas Maybe this is what you have in mind? rerooting_diff.txt

That would keep the same old root node, and simplify things.

jordibc commented 8 months ago

@jhcepas I'm merging this PR and will modify things according to what we talked. Thanks again :)