-
Notifications
You must be signed in to change notification settings - Fork 162
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
base: victorlin/prep-dynamic-yvalues
Are you sure you want to change the base?
Conversation
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. |
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! |
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. |
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 selectedMy 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 Y value paddingIn 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. 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
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: (Perfect is the enemy of good, and I don’t think we need to address this in this PR.) |
82a9f76
to
fe9d629
Compare
fe9d629
to
70e15e4
Compare
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) |
7321e43
to
92b0560
Compare
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 Y-value calculations Focus on selected button/toggle UI:
¹ Adding a "focus on selected" element by itself is clarifying, especially for a v1 design. |
Cool! Thanks so much for picking this up @victorlin. I have a couple comments (similar to James's above) based on UI testing.
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: while a toggle would prevent these funky trees from being accessible. |
@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. |
bc789eb
to
5cdc564
Compare
14f4ac3
to
9ed2880
Compare
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) => { |
There was a problem hiding this comment.
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!):
- We should be able to get rid of
setDisplayOrder
as it's essentially doing the same thing ascalcYValues
and just use one function throughout the code. 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.- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I chose to drop
calcYValues
and add focus functionality tosetDisplayOrder
: 9a2bdb4 + 41ab293 -
Added to checklist:
Disable toggle and turn off focus when using exploded tree / multiple trees in JSON
-
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
6d65167
to
ceec881
Compare
9ed2880
to
7095407
Compare
4dc00f9
to
41ab293
Compare
41ab293
to
d624607
Compare
This allows monitoring of filter changes in changePhyloTreeViaPropsComparison.
609fbe5
to
aa8edd6
Compare
ceec881
to
4658803
Compare
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.
aa8edd6
to
61dbc11
Compare
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.
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 button should be disabled when no filters are appliedbutton removed in favor of sidebar toggleDisable 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:
distanceMeasure
andlayout
to create a new redux variable oftreeZoom
. This is defaults toeven
but is updated tozoom
when clicking the "zoom to selected" tab. Further clicks increment the redux variable tozoom-2
,zoom-3
, etc... and clicking "reset layout" restores it toeven
.calcYValues
function. This dynamically setsnode.n.yValue
based onnode.visibility
, so that calls to other layout functions likerectangularLayout
will have updatednode.n.yValue
from which to constructnode.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 tonode.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:
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.