sriram98v / phylo-rs

Library for phylogenetic trees
MIT License
0 stars 3 forks source link

Tree edit operations (NNI,SPR,TBR) #8

Open sriram98v opened 1 year ago

sriram98v commented 1 year ago

@swagle8987 SPR is implemented for rooted tree. Can NNI and TBR be implemented for and unrooted tree? If so, could you get that part?

swagle8987 commented 1 year ago

Hey @sriram98v for the subtree and prune operations, shouldn't they consume the value of the tree sent in instead of borrowing a mutable reference to self? It would allow us to be more explicit in the fact that these operations are changing the structure of the tree and that the tree after the operation is not the same. If we aren't mutating the original tree then we could just pass a reference anyways instead of sending a mutable reference.

sriram98v commented 1 year ago

@swagle8987 The idea for subtree is to return a copy of the subtree starting at the input node without removing it from self. prune removes the subtree from self (hence using a &mut self) and returns the subtree, without consuming self. Graft consumes the input tree and grafts it to self (mutates self, hence &mut self)

swagle8987 commented 1 year ago

@sriram98v yes but I think it's better if prune and regraft take ownership of the value rather than borrowing a reference. The subtree method can be a "view" method which returns the subtree at the node(although this wouldnt keep track of the changes made within the subtree of the original tree) but the prune and regraft method fundamentally change the tree and hence explicitly for the end user, they consume the original tree. This also avoids bugs since now users don't have to compulsorily keep the tree variable as mutable and can instead create a mutable clone for prune and regraft.