Skip to content

Conversation

@vincerubinetti
Copy link
Member

@vincerubinetti vincerubinetti commented May 20, 2025

Closes #34

  • consistently wrap other components in "card" styling
  • focus style tweaks to other components to make consistent
  • tweak fitViewBox util func params
  • remove unnecessary/vestigial "title" param in sunburst component
  • implement simple tree visualization with d3
  • add fake tree data generation for test bed
  • rearrange order of components on testbed, roughly simplest to most complex
  • incorporate flushSync into useSvgTransform util func for better (in my testing) fitting behavior

@netlify
Copy link

netlify bot commented May 20, 2025

Deploy Preview for molevolvr ready!

Name Link
🔨 Latest commit 6146630
🔍 Latest deploy log https://app.netlify.com/projects/molevolvr/deploys/68364651e2b00500094c3928
😎 Deploy Preview https://deploy-preview-54--molevolvr.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@vincerubinetti
Copy link
Member Author

vincerubinetti commented May 20, 2025

Reviewers, note that the component can also show details for each "intermediate" node on the tree, not just the leaf nodes at the end. To see this, you can just remove the leavesOnly prop where <Tree> is called in Testbed.tsx. Not sure if we would need that in our case, but we get it essentially for free from the architecture of the component.

I also gave each node the ability to have an arbitrary colored "type", since all the other components have that too, but we may not need it. If we keep it, I'll add a <Legend> below the tree as well, like the other components.

Also, the structure of the data that the component takes is the same as that of the sunburst component:

- label: "top level node"
  children:
    - label: "second level node"
      children:
        - label: "leaf node"
# etc...

This may not match the structure of what will come from the backend code computing the phylogeny, but in an effort to keep the component general purpose and reusable, I think it should stay this structure, and we'll eventually have a (hopefully small) bit of frontend code to transform the phylogeny data into this.

@vincerubinetti
Copy link
Member Author

@vincerubinetti vincerubinetti requested a review from epbrenner May 21, 2025 15:10
@vincerubinetti
Copy link
Member Author

I believe this now incorporates what we discussed in the meeting yesterday.

@epbrenner
Copy link

This is a excellent proof of concept for the phylogeny visualization.

I'll provide a couple minor thoughts here.

  1. Measuring behavior is very cool. I notice it's also "measuring" the vertical distance (which is to say the nodes counted in Dist. include the vertical branches), but I'm guessing these vertical branch length values are 0 so it doesn't actually add anything, which is the correct way to do it. Eyeballing it, it does seem that the distance addition logic might be a little off.
Screenshot 2025-05-21 at 9 44 27 AM

This horizontal distance of 2.01 from tip-to-tip doesn't seem to make sense with...
Screenshot 2025-05-21 at 9 45 03 AM
this horizontal distance of 2.09.

These numbers are arbitrary so I could be wrong, but the sum total of horizontal branch separating the first set of points appears much smaller than the horizontal distance separating the second set. It may be summing distance to root of two nodes vs. just the distance between selected nodes, which wouldn't require traversing all the way back to the root for the first picture.

Generally speaking, a user would typically only measure from selected tip-to-tip. In that sense, we wouldn't really need the internal nodes to be their own selectable points, but those are interesting pieces of data to provide at least optionally. We may want to discuss the details of that implementation on our end to give you better guidance, but for the time being, I think only having tip nodes is sufficient, and any internal nodes just won't have the circle shapes designating a selectable node.

  1. For further edge case testing, a lot of the time users might see trees where they have several tips separated by 0 branch length. These are cases where the two representative sequences are identical, so the distance between them is 0. See the green highlighting in this random tree. Most of those nodes have zero difference between them, so they sit on that vertical plank side-by-side. I'm sure your tree can handle this, but I just didn't see it occur in the randomization attempts on refreshing the page, so keep an eye out for that.
Screenshot 2025-05-21 at 9 54 25 AM

In any event, this implementation does what it says and by my estimate, it's ready to merge with further tweaks coming downstream.

Copy link

@epbrenner epbrenner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment for details, but for initial implementation of a scaled phylogenetic tree, I think this hits the mark.

@vincerubinetti
Copy link
Member Author

vincerubinetti commented May 21, 2025

Eyeballing it, it does seem that the distance addition logic might be a little off.

Currently what it does is just sum the dist values for each node in the selected path (shown in green). I'm just using this d3 function to find the path, which goes up the tree (if necessary) and back down to get to the target node.

It sounds like what you might be looking for is this horizontal distance:

Screenshot 2025-05-21 at 2 48 09 PM

In which case, this is very easy to calculate because it'd just be the difference between each node's rootDist value (its distance from the root node), which I'm already calculating for display purposes.

but for the time being, I think only having tip nodes is sufficient, and any internal nodes just won't have the circle shapes designating a selectable node.

We discussed yesterday that we want to show the dist value (and full label, type, etc) for every node (not just tip nodes) in tooltips. As such, we need a big enough hover-target/focus-indicator for accessibility reasons, so we need to keep the circles.

They don't necessarily need to be selectable, though if it doesn't harm anything, I'd rather just leave the selectability in so I don't have to make some special indication to the user that only tip nodes are selectable but any node is hoverable. Plus it still seems like some user might want to measure between two non-tip nodes.

Most of those nodes have zero difference between them, so they sit on that vertical plank side-by-side. I'm sure your tree can handle this, but I just didn't see it occur in the randomization attempts on refreshing the page, so keep an eye out for that.

It should handle this automatically, yes. it would just appear to be a long plank. I've seen this happen with a few nodes, though not a bunch of adjacent ones at the same time.

@vincerubinetti vincerubinetti merged commit 9b446e9 into main Jun 10, 2025
4 checks passed
@vincerubinetti vincerubinetti deleted the tree-viz branch June 10, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tree/dendrogram viz

3 participants