[Biopython-dev] Code review request for phyloxml branch

[Brad Chapman]

Hey Eric;
Happy New Year -- thanks for all the work on TreeIO. This sounds
great and looking forward to getting it in the main trunk. I'd like
to hear Peter's and other's thoughts, but just a few small comments
below.

> The tree annotations (e.g. id) aren't preserved perfectly during conversions
> -- I'll keep working on this, but I don't think it's a blocker. The taxon
> names of terminal nodes are kept as "clade" names in phyloXML for
> round-tripping. Tree topology and branch lengths seem OK.

Are the annotations often used in real life cases or is this more of
a fringe problem? I'm not as familiar with tree work, but know this
is a pain in sequence space. A good goal is to capture the most
common use cases and then integrate the other issues as feasible.

> Bio.Tree.Newick contains simple subclasses of Tree and Subtree, and an
> incomplete set of shims that track Bio.Nexus.Trees.Tree (minus the I/O).
> This is to ease the deprecation and eventual replacement of Bio.Nexus.Trees,
> as I imagine it:
> (1) Port methods from Nexus.Trees to Bio.Tree, simplifying arguments where
> reasonable (since the node IDs and adjacency list lookup are no longer
> needed)
> (2) Implement methods in Bio.Tree.Newick with the original argument lists,
> but triggering a deprecation warning indicating the newer replacement method
> (3) Replace Nexus.Trees with an import of Bio.Tree.Newick(IO) and a few more
> shims to duplicate the original API -- so test_Nexus.py should still pass,
> ideally (with deprecation warnings)
> (4) In Nexus.Nexus, replace all usage of Nexus.Trees with proper usage of
> NexusIO and Bio.Tree methods.
> (5) Eventually delete Nexus.Trees and the shims in Bio.Tree.Newick.
> 
> I'm currently doing (1) and (2), with more emphasis on getting (1) right.
> Not all of the important methods have been ported, but I'm happy with the
> tree traversal methods.

Nice. This all sounds like a really good refactoring. It sounds like 1 
can happen once this all gets merged with the main branch, and
could benefit from others being able to more easily look at it and
make suggestions.

> I noticed that in Tests/Nexus/, the example file for internal node labels is
> actually in Newick/NH format, not Nexus. That was briefly confusing, so
> maybe that file should be renamed.

Oops, I think that may have been me. No problem, rename away.

Brad