Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tree: Add toggle to focus on selected #1373

Open
wants to merge 4 commits into
base: victorlin/prep-dynamic-yvalues
Choose a base branch
from

Conversation

trvrb
Copy link
Member

@trvrb trvrb commented Jul 11, 2021

preview on ncov/gisaid/global/all-time

Description of proposed changes

This PR adds a toggle in the sidebar to emphasize visible nodes by expanding them to take up 80% of the vertical span of the panel.

focus-demo.mov

Related issue(s)

#1368

Design decisions

Known bugs to fix

  • Focus should update with filter changes and zoom level changes
  • Focus button should be disabled when no filters are applied button removed in favor of sidebar toggle
  • Focus should only be re-calculated on filter and zoom changes (ref)
  • Y-value calculations should be updated to work better when only a few samples are selected (James: it's to do with the asymmetric allocation of space either side sets of selected tips - should be symmetric) Done: a1ae090
  • "Reset layout" should properly update branch thickness when focus is enabled
  • Add info on-hover help box to sidebar
  • Add experimental icon
  • Move toggle higher in sidebar
  • Hide toggle (but ideally keep state) when not in rectangular or radial tree layouts
  • Disable toggle and turn off focus when using exploded tree / multiple trees in JSON (not necessary, it is working well)

Other tasks

Original PR description

This commit re-implements the "zoom to selected" function in the tree panel to emphasize visible nodes by expanding their "yValues" to take up 80% of the vertical span of the panel. Notes on implementation details:

  • I mirrored redux dataflow of distanceMeasure and layout to create a new redux variable of treeZoom. This is defaults to even but is updated to zoom when clicking the "zoom to selected" tab. Further clicks increment the redux variable to zoom-2, zoom-3, etc... and clicking "reset layout" restores it to even.
  • A PhyloTree redraw is triggered when redux treeZoom variable is updated. This allows filters to change, etc... without triggering immediate changes to layout, but then clicking "zoom to selected" will redraw layout to emphasize currently selected nodes.
  • phylotree.layouts contains the actual logic in the calcYValues function. This dynamically sets node.n.yValue based on node.visibility, so that calls to other layout functions like rectangularLayout will have updated node.n.yValue from which to construct node.y.

This is mostly working at this point, though there are a few issues (like initial render with ?z=zoom in URL does not work as it should due to node.visibility not existing in the initial calls to layout). I'm hoping first for user interface feedback (hence the draft PR).

Primary use case is to:

  1. Filter to a trait value of interest (whether monophyletic or not)
  2. Click "zoom to selected" to emphasize nodes with trait.

The biggest wins are being able to zoom to basal tips in the tree. For example, you can zoom to early SARS-CoV-2 samples or to clade 19A where this wasn't previously possible.

@jameshadfield jameshadfield temporarily deployed to auspice-dynamic-yvalues-mqlvu0 July 11, 2021 22:43 Inactive
@huddlej
Copy link
Contributor

huddlej commented Jul 12, 2021

I haven't had a chance to do a full review yet, but at first glance, this is a really cool feature! It seems pretty helpful for situations like H3N2 where we have recurrent mutations like 186D and we want to focus on these.

@emmahodcroft
Copy link
Member

I haven't tested this out super-extensively yet, but I tried out zooming to 19B (currently can be quite difficult!) and also picking out a few sequences at the 'edges' of clades (which can also be extremely difficult to get a good look at, if they don't cluster but branch sequentially away from the clade). Both worked beautifully and gave super useful views that I don't think you can get with current auspice!

@joverlee521
Copy link
Contributor

It's really cool that the zoom works well with the date filter! 🤩

I noticed that the interaction between zooming via branches and the zoom button can be a little clunky.

  1. Zoom in via a branch:
    Screen Shot 2021-07-13 at 5 13 32 PM

  2. Then clicking on the zoom button brings up previously hidden branches on the bottom, making the x-axis hard to read:
    Screen Shot 2021-07-13 at 5 13 55 PM

  3. Zooming back out via clicking on base branches leaves the tree in a strange configuration:
    Screen Shot 2021-07-13 at 5 15 12 PM

@jameshadfield
Copy link
Member

The general behaviour is fantastic. I’m really happy to see the y-value-zoom functionality resurrected in this (much improved) form! Here’s a longer review of the functionality (I haven’t dug into the code at all yet, but I will).

Zoom to selected

My main issue is the confusing UI due to us mixing the y-zoom behaviour with the “zoom to selected button”.

(Overview of how this button currently works: it zooms into the MRCA of the selected tips - using the terminology of Auspice we change the in-view root node to be the MRCA of the tips which are both visible and in-view. If this node is the same as the current in-view root node, then the button isn’t selectable. Note that this MRCA node may not be visible, and thus you can’t always achieve this behaviour by clicking on the branch to zoom.)

Importantly, “zoom to selected” isn’t a flag or a state, it’s more like an action which is conditional on other state; this is why it’s so hard to store it as a URL query (see #1321).

With this PR, it’s really easy to get into intermediate states. For instance filter to Oceania, then zoom-to-selected, then inactivate the filter. The y-zoom is still focused on Oceania, but all the tips are visible again. There are plenty of other ones too (e.g. Jover’s screenshots), and it’s not clear when you’re in one of these intermediate states.

An alternative UI approach would be a “Focused zoom” toggle. With this on, any change in visibility (zooming, filtering etc) would trigger a recalculation of y values. This would look really nice when changing filters or animating (if it could be made fast enough). I think this UI would avoid all the intermediate states currently available in this PR, and make it extremely clear what mode we are currently in. It’s somewhat analogous to separating out the horizontal and vertical zooming.

Y value padding

In certain situations, (e.g. South America + 614D) the y-value calculations aren’t right. It looks like we’re setting some padding above selected tips, but not below them, which means the squashed branches are directly behind selected tips.

image

Do we still need the (existing) “zoom-to-selected” behaviour?

Perhaps not. I haven’t fully thought through all the use cases here, but I think we could remove this. We can still zoom into clades by clicking on branches to explore certain parts of the tree, which is essentially what the button does.

Zooming into early samples

The biggest wins are being able to zoom to basal tips in the tree.

This is much improved in this PR, but there are still some cases it doesn’t solve, as the upper bound of the time domain is always the latest date of the in-view tips. For instance, zooming into clade 1B of flu/seasonal/vic/ha/12y could be better:

image

(Perfect is the enemy of good, and I don’t think we need to address this in this PR.)

@trvrb trvrb temporarily deployed to auspice-dynamic-yvalues-jxzjzp July 22, 2021 00:08 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-n9rt9e September 13, 2024 18:22 Inactive
@victorlin
Copy link
Member

victorlin commented Sep 13, 2024

Not able to build a new review app due to old NPM version. Rebase seems mostly trivial so I'm going to do that and hopefully get a working review app. Original commit is e4b02c5 if we want to revert back.

EDIT: I believe more work needs to be done to account for ca1046b and 00add58
EDIT: done

@nextstrain-bot nextstrain-bot temporarily deployed to auspice-dynamic-yvalues-76vqio September 13, 2024 18:35 Inactive
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 18:39 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 21:03 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 21:16 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 21:27 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 22:05 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 22:27 Inactive
@victorlin victorlin self-assigned this Sep 13, 2024
@victorlin
Copy link
Member

PR is working now. I've updated the description with a link to a nextstrain.org preview instance and some remaining tasks. Also added some broader thoughts in the linked issue: #1368 (comment)

@jameshadfield
Copy link
Member

jameshadfield commented Oct 6, 2024

I'm really excited about this functionality, as well as the next steps of x-zoom! Here's some comments after using the UI for ~30min, but I haven't looked at the code.

URL query
Let's disable the URL query for the first implementation. We've generally followed a pattern of testing a feature out for a while before adding the query - especially so for new functionality like this. Stepping back a bit, this relates to my comment above and also referenced in this related issue regarding a future where we drop the "zoom to selected" and just have focus. Or maybe vice-versa!

Y-value calculations
The y-value calculations are still not quite right - I think this is unchanged from my comment above. Using a filter with few samples makes this very obvious. Here's a screenshot from a subtree of Oropouche filtered to n=2 Columbia samples:

image

Focus on selected button/toggle UI:

  1. The element looks to be clickable when a tree (with no filters) is first loaded, but it does nothing
  2. How can we "disable" focus -- the element still appears to be clickable and says "focus on ..." and clicking it does nothing. Clicking "reset layout" restores the original view, but this wasn't obvious to me.
  3. Is this supposed to act like a toggle or a button? I think we're aiming for the former¹. Assuming so, as we zoom the tree (clicking on branches / "zoom to selected" button) the y-values need to recalculate as different samples enter the viewport but they currently don't. Here's a screenshot of the above oropouche example where the focus was applied with n=2 samples selected and then I've zoomed out so now n=6 are selected but the new 4 are all squashed down the bottom (LHS) - the correct behaviour is on the RHS:
image 2 4. Assuming it is a toggle, the element should have a little on/off toggle or similar to convey this.
  1. Inactivating / disabling a filter doesn't re-calculate the y-value (when "focused") -- this is the case I referred to in my original review about "intermediate states".

¹ Adding a "focus on selected" element by itself is clarifying, especially for a v1 design.

@trvrb trvrb temporarily deployed to auspice-dynamic-yvalues-iqtpul October 7, 2024 22:06 Inactive
@trvrb
Copy link
Member Author

trvrb commented Oct 7, 2024

Cool! Thanks so much for picking this up @victorlin. I have a couple comments (similar to James's above) based on UI testing.

  1. I don't like adding things as options to the top right of a panel unless it's really something that should be an immediate click away (like "reset"). I'd suggest keeping this as a real toggle in the sidebar. It would act in a similar way as "Show confidence intervals" as changing the continuous behavior of the tree. I'd place this toggle at the end below "Explode Tree By" and I'd give it a similar experimental icon. For text I'd suggest "Focus tree on selected nodes".

  2. Currently, the y-values aren't getting updated after new nodes are selected. To demonstrate go to https://nextstrain-s-nextstrain-l8gysc.herokuapp.com/zika?f_country=Colombia and then "Toggle focus" this updates y-values to focus on Colombia tips. However, then if you remove the Colombia filter and/or apply a USVI filter the y-values don't update. In "focus mode" the y-values should always reflect active tips.

I think that overall I recommend a "toggle" behavior and placing the toggle in the sidebar. I could see an argument for making it a button rather than a toggle ("focus on selected") that's essentially the current behavior. In this case I think it should really be possible to combine this button with the "zoom to selected" button. However, removing a filter with zoomed y-values results in a funky looking tree, like so:

Screenshot 2024-10-07 at 3 20 08 PM

while a toggle would prevent these funky trees from being accessible.

@victorlin
Copy link
Member

@jameshadfield @trvrb thanks for the feedback! Switched to toggle behavior in fa421d5. PR description has been updated with a list of the clear bugs to tackle and open design decisions.

I've also done a bit of extra refactoring and will make a base PR with changes that can be considered before the rest of this PR is ironed out.

@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 8, 2024 23:28 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 9, 2024 00:16 Inactive
@victorlin
Copy link
Member

Everything but the Y calculation bug have been addressed. I'll look more into that tomorrow.

* rectangularLayout, radialLayout, createChildrenAndParents
* side effects: node.displayOrder and node.displayOrderRange (i.e. in the phyloTree node)
*/
export const calcYValues = (nodes, focus) => {
Copy link
Member

Choose a reason for hiding this comment

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

Brief comments (after looking at the code for perhaps the first time since I wrote the original prototype!):

  1. We should be able to get rid of setDisplayOrder as it's essentially doing the same thing as calcYValues and just use one function throughout the code.
  2. setDisplayOrder specifically considers the case of an exploded tree which made me wonder what happens if we explode a tree and then toggle on focus - it's a bug! We should test the similar case of a JSON with n>1 trees (subtrees). In either case, for v1 functionality we can disable the focus functionality when we've exploded a tree / have multiple subtrees in the JSON.
  3. Now that this is called frequently (rather than simply on dataset load) we can probably make it faster if needed. I see setTreeFocus has timers in it - we should build Auspice with --includeTiming and see how fast/slow this currently is.

Copy link
Member

@victorlin victorlin Oct 10, 2024

Choose a reason for hiding this comment

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

  1. I chose to drop calcYValues and add focus functionality to setDisplayOrder: 9a2bdb4 + 41ab293

  2. Added to checklist:

    Disable toggle and turn off focus when using exploded tree / multiple trees in JSON

  3. I did this locally and through some spot checking only saw Timer setTreeFocus (#XX) took 0ms. Average: 0ms.. Maybe I'm not checking in the right places though. Maybe we can tweak the review app to build with --includeTiming?

Copy link
Member

Choose a reason for hiding this comment

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

Disable toggle and turn off focus when using exploded tree / multiple trees in JSON

I fixed a bug (not sure if it's the same one that you saw) and it doesn't look too bad:

focus-exploded-tree.mov

Maybe we can allow focus on multiple trees? Unless there is another bug that I missed.

Copy link
Member

Choose a reason for hiding this comment

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

That looks great! Yes ideally we'd allow it, I just wanted to convey it wasn't essential for the v1.

src/components/controls/toggle-focus.tsx Outdated Show resolved Hide resolved
src/components/controls/toggle-focus.tsx Show resolved Hide resolved
src/components/tree/phyloTree/layouts.js Outdated Show resolved Hide resolved
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 9, 2024 23:53 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 10, 2024 00:22 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 10, 2024 00:32 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 10, 2024 21:41 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 10, 2024 22:11 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 10, 2024 22:15 Inactive
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 10, 2024 22:19 Inactive
@victorlin victorlin marked this pull request as ready for review October 10, 2024 22:19
This allows monitoring of filter changes in
changePhyloTreeViaPropsComparison.
trvrb and others added 2 commits October 11, 2024 10:40
This commit adds a toggle in the sidebar to emphasize visible nodes by
expanding them to take up 80% of the vertical span of the panel.
@victorlin victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 11, 2024 17:41 Inactive
@victorlin victorlin changed the title Vertically focus on filtered nodes tree: Add toggle to focus on selected Oct 11, 2024
This function is a good candidate for timing: it recurses through all
tree nodes and is called upon initial load and every update of filters
or zoom level.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

8 participants