Newick/Nexus processing of non-binary trees

[Tiago Antão]

Hi,

Either I am looking for too much time to the code or it seems to me
that the current implementation only supports binary trees (ie, trees
with 2 children).

I have tested with:
tree tree6 = (1,2,3);

And I get only 2 edges. The edge pointing to "1" gets lost.
Inspecting the old code, this seems to be how it is implemented.

In the case I am correct, this renders the whole tree parser somewhat
useless in its current form, as most phylo trees are not binary only.

The other two bugs are now corrected, but this is much more serious, me thinks.

--
"The hottest places in hell are reserved for those who, in times of
moral crisis, maintain a neutrality." - Dante
_______________________________________________
Biojava-l mailing list  -  Biojava-l@...
http://lists.open-bio.org/mailman/listinfo/biojava-l

[Richard Holland-6]

If that's true, sounds like it's broke. Is the old code easily  
modified to suit arbitrary numbers of children?

On 6 Nov 2009, at 13:26, Tiago Antão wrote:

> Hi,
>
> Either I am looking for too much time to the code or it seems to me
> that the current implementation only supports binary trees (ie, trees
> with 2 children).
>
> I have tested with:
> tree tree6 = (1,2,3);
>
> And I get only 2 edges. The edge pointing to "1" gets lost.
> Inspecting the old code, this seems to be how it is implemented.
>
> In the case I am correct, this renders the whole tree parser somewhat
> useless in its current form, as most phylo trees are not binary only.
>
> The other two bugs are now corrected, but this is much more serious,  
> me thinks.
>
> --
> "The hottest places in hell are reserved for those who, in times of
> moral crisis, maintain a neutrality." - Dante
> _______________________________________________
> Biojava-l mailing list  -  Biojava-l@...
> http://lists.open-bio.org/mailman/listinfo/biojava-l

--
Richard Holland, BSc MBCS
Operations and Delivery Director, Eagle Genomics Ltd
T: [phone] ext 3 | E: holland@...
http://www.eaglegenomics.com/


_______________________________________________
Biojava-l mailing list  -  Biojava-l@...
http://lists.open-bio.org/mailman/listinfo/biojava-l

[Tiago Antão]

2009/11/6 Richard Holland <holland@...>:
> If that's true, sounds like it's broke. Is the old code easily modified to
> suit arbitrary numbers of children?

I don't think so. It uses a stack based solution, so it would not be
possible to know when a part of the stack belongs to the current node
being processed or something else on the tree.
One could put markers on the stack or something, but it would become a
bit convoluted. I would suppose a recursive implementation would be
cleaner here.

My suggestion: for somebody else to verify my findings. I might be
doing something stupidly wrong. Maybe things are correct. Just a
simple tree like (1,2,3) (as long as it is not binary) - should expose
the problem.
_______________________________________________
Biojava-l mailing list  -  Biojava-l@...
http://lists.open-bio.org/mailman/listinfo/biojava-l

[Tiago Antão]

Hi,

> My suggestion: for somebody else to verify my findings. I might be
> doing something stupidly wrong. Maybe things are correct. Just a
> simple tree like (1,2,3) (as long as it is not binary) - should expose
> the problem.
>

Has nobody answered here is my take:

1. The error reported probably exists
2. Most probably nobody is using the parser (as it only supports binary trees).

In this light, changing the API should not be a problem at all.

I would not mind correcting the problem (I have already corrected the
previous 2 ones in my local version).
I would suggest removing the call to the unweighted graph. Reasons:
1. A weighted version is enough. If branch lengths are not specified,
then weights could be set to 0. There there would not be a decrease in
functionality.
2. Severely reducing the size of the code is important. Clearly the
code is not much maintained (and I am not offering to maintain it in
the long run, just putting it in good shape) and not much used.
Therefore a smaller, more easy to manage code base makes even more
sense.

If you accept a solution along these lines. I would correct all the
bugs and also include test code (which is also missing).



--
"The hottest places in hell are reserved for those who, in times of
moral crisis, maintain a neutrality." - Dante
_______________________________________________
Biojava-l mailing list  -  Biojava-l@...
http://lists.open-bio.org/mailman/listinfo/biojava-l

[Richard Holland-6]

i'm all for that.

The original code was developed by a Google Summer of Code student, who we haven't heard much from since. :(

cheers,
Richard

On 13 Nov 2009, at 12:25, Tiago Antão wrote:

> Hi,
>
>> My suggestion: for somebody else to verify my findings. I might be
>> doing something stupidly wrong. Maybe things are correct. Just a
>> simple tree like (1,2,3) (as long as it is not binary) - should expose
>> the problem.
>>
>
> Has nobody answered here is my take:
>
> 1. The error reported probably exists
> 2. Most probably nobody is using the parser (as it only supports binary trees).
>
> In this light, changing the API should not be a problem at all.
>
> I would not mind correcting the problem (I have already corrected the
> previous 2 ones in my local version).
> I would suggest removing the call to the unweighted graph. Reasons:
> 1. A weighted version is enough. If branch lengths are not specified,
> then weights could be set to 0. There there would not be a decrease in
> functionality.
> 2. Severely reducing the size of the code is important. Clearly the
> code is not much maintained (and I am not offering to maintain it in
> the long run, just putting it in good shape) and not much used.
> Therefore a smaller, more easy to manage code base makes even more
> sense.
>
> If you accept a solution along these lines. I would correct all the
> bugs and also include test code (which is also missing).
>
>
>
> --
> "The hottest places in hell are reserved for those who, in times of
> moral crisis, maintain a neutrality." - Dante

--
Richard Holland, BSc MBCS
Operations and Delivery Director, Eagle Genomics Ltd
T: [phone] ext 3 | E: holland@...
http://www.eaglegenomics.com/


_______________________________________________
Biojava-l mailing list  -  Biojava-l@...
http://lists.open-bio.org/mailman/listinfo/biojava-l

[Thasso Griebel]

Hi,

> 1. A weighted version is enough. If branch lengths are not specified,
> then weights could be set to 0. There there would not be a decrease in
> functionality.

just my two cents, but I would go with a default weight of 1.0. If you  
read something unweighted you would ignore the edge weights anyways,  
but, for example, if you write something simple that computes path  
lengths, a default weight of 1.0 ensures that the method also works  
for "unweighted" trees, where the length of a path is defined as the  
number of edges you need to traverse to move from say A to B. I think  
the argument also hold for other algorithms used on trees and graphs.

anyways, just my two cent.

-thasso

--
Dipl. Inf. Thasso Griebel-------------------Lehrstuhl fuer Bioinformatik
Office 3426--http://bio.informatik.uni-jena.de--Institut fuer Informatik
Phone [phone]-----------Friedrich-Schiller-Universitaet Jena
Fax [phone]----------Ernst-Abbe-Platz 2, 07743 Jena, Germany



_______________________________________________
Biojava-l mailing list  -  Biojava-l@...
http://lists.open-bio.org/mailman/listinfo/biojava-l

[Tiago Antão]

2009/11/13 Thasso Griebel <thasso.griebel@...>:
> just my two cents, but I would go with a default weight of 1.0. If you read
> something unweighted you would ignore the edge weights anyways, but, for
> example, if you write something simple that computes path lengths, a default
> weight of 1.0 ensures that the method also works for "unweighted" trees,
> where the length of a path is defined as the number of edges you need to
> traverse to move from say A to B. I think the argument also hold for other
> algorithms used on trees and graphs.

OK, I will do this.
_______________________________________________
Biojava-l mailing list  -  Biojava-l@...
http://lists.open-bio.org/mailman/listinfo/biojava-l