[Bioperl-l] Unwise elimination of nodes inB:T:Node::remove_Descendent?

[Mark A. Jensen]

np-

(note that no commits have occurred yet on this.)
MAJ comments under Bug #2456
****
I've identified (and I think fixed) the problems. I will paste in my reworked
reroot() with my comments and editorializing to myself here (these won't appear
in any commits) to indicate the issues.

I've also written a new helper function for B:T:Node called
create_node_on_branch(), which figures in the fix below. I'll describe it
briefly in a separate comment.

The clue that broke this case was that reroot()'ing on a leaf (as Stephanie
did) gave the wrong answer (according to close visual inspection of the tree as
rendered in FigTree), while reroot()'ing on an internal node gave the right
answer.

The main issue in the original reroot was the unnecessary creation of new node
at the midpoint of the new root's ancestral branch that got left in the tree
after rerooting. The original author felt that rerooting on a leaf was a
special case, but the case wasn't handled correctly, so that an extra branch
got created in the tree, and a spurious root that was not the desired new root
(i.e., not the leaf requested by the user) was created.

[see the code at http://bugzilla.open-bio.org/show_bug.cgi?id=2456#c3]

***
create_node_on_branch() separates the issue of rerooting from the issue of
deciding whether a new root should be created in the middle of a branch. These
issues seemed to be conflated in the original reroot. Suppose now one wants to
create a new root for a tree between the nodes A -> B at the midpoint between A
and B. The code now would be:

 my $newroot = $tree->find_node('B')->create_node_on_branch(-FRACTION=>0.5,
     -ANNOT=>{-id=>'midpt',-desc=>'new midpoint root'});
 $tree->reroot($newroot);

Before you would (prob) have to do this 'by hand'--

 my $nodeB = $tree->find_node('B');
 my $nodeA = $tree->find_node('A');
 my $newnode = Bio::Tree::Node->new(-branch_length=>0.5*$nodeB->branch_length);
 $newnode->ancestor($nodeA);
 $newnode->add_Descendent($nodeB);
 $nodeA->remove_Descendent($nodeA);
 $nodeB->branch_length(0.5*$nodeB->branch_length);
 # then
 $tree->reroot($newnode);
 # and hope for the best...
***
Going thru Tree.t-- I see there is a real conflation of rerooting and root
*creation*. The current (old) implementation creates an extra node and makes it
the root, which isn't what I would expect -- if you reroot on a node, you want
that very node to be the root. If you want a new root in my scheme, you create
that node on the branch where you want and then reroot to that node. I think
this is more natural, but maybe not for everyone. Anyway, I'm modifying the
test file and will throw up a patch later.
***
The mods to the other tests are mainly related to a miscounting of nodes for a
set of related trees; get_nodes is right and the test is wrong, I believe. I've
uploaded pdfs of FigTree renderings of the trees in question for others to
doublecheck.
***
May I take a moment here to say that FigTree rocks.
***
FigTree is at http://tree.bio.ed.ac.uk/software/figtree/ , btw.


----- Original Message ----- 
From: "Hilmar Lapp" <hlapp at gmx.net>
To: "Mark A. Jensen" <maj at fortinbras.us>
Cc: "Chris Fields" <cjfields at illinois.edu>; <bioperl-l at lists.open-bio.org>
Sent: Saturday, February 07, 2009 4:12 PM
Subject: Re: [Bioperl-l] Unwise elimination of nodes 
inB:T:Node::remove_Descendent?


>
> On Feb 7, 2009, at 3:39 PM, Mark A. Jensen wrote:
>
>> editorial analysis up under Bug #2456
>
>
> Would you mind copying that here? This list is probably best for  discussing 
> desired behavior and related issues, if that's what the  editorial analysis is 
> about.
>
> -hilmar
> -- 
> ===========================================================
> : Hilmar Lapp  -:-  Durham, NC  -:-  hlapp at gmx dot net :
> ===========================================================
>
>
>
>
>