jeetsukumaran / DendroPy

A Python library for phylogenetic scripting, simulation, data processing and manipulation.
https://pypi.org/project/DendroPy/.
BSD 3-Clause "New" or "Revised" License
210 stars 61 forks source link

bipartition.is_trivial is True for non-trivial bipartitions in v4.0.3 #27

Closed daveuu closed 9 years ago

daveuu commented 9 years ago

In the following print() output, I would not expect line 3 to be included because AB | CD isn't a trivial bipartition:

:tree = dendropy.Tree.get_from_string('((A,B),(C,D));', 'newick', rooting=None)
:tree.encode_bipartitions()
:for bipartition in tree.bipartition_encoding:
:    if bipartition.is_trivial:
:        print(bipartition.split_as_newick_string(tree.taxon_namespace))
:
:--                               
((B, C, D), (A));
((B), (A, C, D));
((C, D), (A, B));
((C), (A, B, D));
((D), (A, B, C));
(A,B,C,D);

I also wouldn't expect the last line to be printed because "rooting=None" so it is partitioning an unrooted tree from nothing. Is this behaviour unexpected?

jeetsukumaran commented 9 years ago

(1) There is a bug in your code: Bipartition.is_trivial is a function, not a property or attribute. Perhaps it makes sense for it to be the latter, but since some calculations go on behind the scenes to determine this, keeping it as a function alerts the user (or me, at least, when using it) that the call is not without cost.

The line:

if bipartition.is_trivial

will always return True. It should be:

if bipartition.is_trivial()

(2) As to the fact that "(A,B,C,D)" is considered a bipartition, I can see why this might be objectionable. Bipartitions or splits correspond to edges, and while a rooted tree might have an edge for the root, it does not make sense for an unrooted tree, where the root is an algorithmic artifact.

However, when dealing with trees with incomplete leafsets, it is useful to have a bipartition available for inspection/comparison at the root, to quickly assess which leaves are present in the tree, whether or not the tree is rooted. So corresponding to the algorithmic convenience (or necessity) of having a "root" in an unrooted tree, we also find it algorithmically convenient to have a bipartition corresponding to the initial split in an unrooted tree in the cases of dealing with trees with incomplete leaf sets.

-- jeet

On 8/21/15 2:08 PM, daveuu wrote:

In the following print() output, I would not expect line 3 to be included because AB | CD isn't a trivial bipartition:

|:tree = dendropy.Tree.get_from_string('((A,B),(C,D));', 'newick', rooting=None) :tree.encode_bipartitions() :for bipartition in tree.bipartition_encoding: : if bipartition.is_trivial: : print(bipartition.split_as_newick_string(tree.taxon_namespace)) : :-- ((B, C, D), (A)); ((B), (A, C, D)); ((C, D), (A, B)); ((C), (A, B, D)); ((D), (A, B, C)); (A,B,C,D); |

I also wouldn't expect the last line to be printed because "rooting=None" so it is partitioning an unrooted tree from nothing. Is this behaviour unexpected?

— Reply to this email directly or view it on GitHub https://github.com/jeetsukumaran/DendroPy/issues/27.


Jeet Sukumaran

jeetsukumaran@gmail.com

Blog/Personal Pages: http://jeetworks.org/ GitHub Repositories: http://github.com/jeetsukumaran Photographs (as stream): http://www.flickr.com/photos/jeetsukumaran/ Photographs (by galleries):

http://www.flickr.com/photos/jeetsukumaran/sets/

daveuu commented 9 years ago

That makes sense, thanks.