#1008 split tree and group into subtrees by state #1105
Conversation
… yet implemented)
…rait and regular (still slow though)
…ups or hiding branches
frogsquire
changed the title
|
@jameshadfield @evogytis Just wanted to check in on this -- any thoughts? At a point where I want to be sure this is a good direction before doing more. |
|
Hey @frogsquire, I checked the branch as of 2870aa3 and this is what I get (this is DENV1 dataset): What I'm seeing is missing internal nodes. This is what it should look like roughly (colours and sorting might differ, but the look of each subtree shouldn't): |
|
Very exciting to see this progress @frogsquire. 😀 I see the same issue as Gytis with deeper nodes missing connections. I think for ordering subtrees I would do:
|
| { | ||
| args.splitTreeByTrait = newProps.tree.splitTreeByTrait; | ||
| if (args.splitTreeByTrait == null) {// if the split is being unset, reset the layout | ||
| // todo: could use updateLayout instead? |
There was a problem hiding this comment.
Let's revisit this, and exactly how we're using d3 to update the DOM (e.g. transitions) once the algorithm is working correctly
There was a problem hiding this comment.
Update on this - the d3 transition is working reasonably well for turning the toggle off. For turning it on, it still looks a bit jerky.
There was a problem hiding this comment.
@jameshadfield, if you feel we are at this point, do you have any thoughts on why the transition is still not smooth when enabling the toggle (though it is smooth at all other times)?
There was a problem hiding this comment.
I'll look into this shortly, as I do think we're at this point 😄
…e all children before pushing node onto subtree stack to improve ordering
…t trait data; unhide them when turning split mode off
|
@jameshadfield Please see the attached image diffs from the integration tests. The only differences I can see is that there's some issues with font aliasing between my machine and the reference images, and that the UI has been altered because of the new toggle. If these look acceptable to you, let me know, and I can work on the font issue (according to this and the terminal output I've gotten, I may just need to reinstall some fonts) and provide new reference images.
|
|
Base changed and changed back because I ran into this issue. |
Introduces the ability to cut trees at various branches and display the resulting trees as subtrees. These cut points are here defined to be a change in a (user-selected) state trait, which are simply the non-continuous colourings (to start with). Briefly, given a parent node (A) with children (B,C), and a change in trait moving from (A->B), we add B to the children of the overall tree root, and update the children of A to be only C. We store the original children (B,C) so that we can return to the starting state. This implementation, specifically the ability for PhyloTree to update the node ordering, should allow for branch rotations in the future. There are a number of future improvements to be made in subsequent commits: * The subtrees should be ordered on trait (or we may wish to make this a toggle) * the stem length of subtrees is inconsistent * exploded trait cannot be stored in URL state * we should limit the explode options to those colourings which are defined on internal nodes (currently a warning is shown) * genotype should be available to explode on * remove console logging introduced here * consider undefined (internal) traits, e.g. currently nodes (X -> undefined -> Y) would not be exploded * multiple trees are not considered here Prior art: I was first made aware of this style of tree rendering by Gytis Dudas. An implementation was attempted by @frogsquire<https://github.com/frogsquire> in #1105 and this commit uses ideas introduced there.
Introduces the ability to cut trees at various branches and display the resulting trees as subtrees. These cut points are here defined to be a change in a (user-selected) state trait, which are simply the non-continuous colourings (to start with). Briefly, given a parent node (A) with children (B,C), and a change in trait moving from (A->B), we add B to the children of the overall tree root, and update the children of A to be only C. We store the original children (B,C) so that we can return to the starting state. This implementation, specifically the ability for PhyloTree to update the node ordering, should allow for branch rotations in the future. There are a number of future improvements to be made in subsequent commits: * The subtrees should be ordered on trait (or we may wish to make this a toggle) * the stem length of subtrees is inconsistent * exploded trait cannot be stored in URL state * we should limit the explode options to those colourings which are defined on internal nodes (currently a warning is shown) * genotype should be available to explode on * remove console logging introduced here * consider undefined (internal) traits, e.g. currently nodes (X -> undefined -> Y) would not be exploded * multiple trees are not considered here Prior art: I was first made aware of this style of tree rendering by Gytis Dudas. An implementation was attempted by @frogsquire<https://github.com/frogsquire> in #1105 and this commit uses ideas introduced there.
Introduces the ability to cut trees at various branches and display the resulting trees as subtrees. These cut points are here defined to be a change in a (user-selected) state trait, which are simply the non-continuous colourings (to start with). Briefly, given a parent node (A) with children (B,C), and a change in trait moving from (A->B), we add B to the children of the overall tree root, and update the children of A to be only C. We store the original children (B,C) so that we can return to the starting state. This implementation, specifically the ability for PhyloTree to update the node ordering, should allow for branch rotations in the future. There are a number of future improvements to be made in subsequent commits: * The subtrees should be ordered on trait (or we may wish to make this a toggle) * the stem length of subtrees is inconsistent * exploded trait cannot be stored in URL state * we should limit the explode options to those colourings which are defined on internal nodes (currently a warning is shown) * genotype should be available to explode on * remove console logging introduced here * consider undefined (internal) traits, e.g. currently nodes (X -> undefined -> Y) would not be exploded * multiple trees are not considered here Prior art: I was first made aware of this style of tree rendering by Gytis Dudas. An implementation was attempted by @frogsquire<https://github.com/frogsquire> in #1105 and this commit uses ideas introduced there.
Outstanding issues I am still working on:
Some other points following up on our discussion:
num_datetrait. This seems to work quite well visually, I think.-- I couldn't find a way to run this code without updating the layout, and without using the
modifySVGInStagesapproach. This makes the transition when flipping the checkbox on and off pretty slow. Would love to improve this.-- Any optimizations of the tree traversal/subtree data gathering process would be appreciated.
-- I need to do some code cleanup/quality improvements (for example, using a dictionary or trait-indexed array instead of a custom object to store the subtrees), run the linter, etc. Would appreciate any suggestions on this front as well.
I will continue working on this over the coming days, but wanted to update on my progress again. Thanks!