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

Temporal visibility #639

Merged
merged 4 commits into from
Aug 31, 2018
Merged

Temporal visibility #639

merged 4 commits into from
Aug 31, 2018

Conversation

jameshadfield
Copy link
Member

@jameshadfield jameshadfield commented Aug 25, 2018

This PR changes the temporal branch filtering to only highlight branches whose end-points (tips for terminal nodes) are within the current time slice (previously it was highlighted if any time point between the tip and the parent node intersected with the selected time slice).

Before:
image

After:
image

This isn't perfect, and doesn't address all the concerns in #473, but I think it's an improvement.

@jameshadfield jameshadfield requested review from trvrb and rneher August 25, 2018 01:43
@trvrb
Copy link
Member

trvrb commented Aug 25, 2018

I agree with the intention, but I don't think this works as implemented. It breaks the logic of how we calculate map transitions. Compare:

http://localhost:4000/zika?dmax=2014-05-11&dmin=2013-08-13
https://nextstrain.org/zika?dmax=2014-05-11&dmin=2013-08-13

You can see that the transition from Brazil to Haiti is definitely in the window. However, on the PR branch the map does not show this transition.

Consequently, if you click to play the animation, it appears quite broken. Same for Ebola.

I think this requires deeper changes to get working.

@jameshadfield
Copy link
Member Author

jameshadfield commented Aug 25, 2018

Hmm, yeah the map animation looks much worse with this. But I also think the tree, as it currently stands, is extremely misleading (e.g. the first screenshot).

So in that time slice, there's a branch from an ancestral state of Dom. Rep. (which is within the selected time slice) to Haiti (outside of the slice). The migration event could have occurred anywhere on that branch (and included unsampled intermediates). For the tree, I think this PR is better than the current release, i.e. this branch shouldn't be displayed. But for the For the map, where we infer a gradual missile from Dom. Rep to Haiti over the branch length, this PR is terrible.

I wonder if a solution which changes the visibility array from containing hidden & visible strings to containing {0, 1, 2}, where 0 is out-of-selection, 1 is display on map, and 2 is display on map and tree would be an acceptable solution. This should preserve the tree behavior of this PR together with the current live behavior of the map.

One alternative would be to move the map slicing of these "partially in-selection" branches to redux state and changing the tree to draw those branches as partially in-selection, which is much more involved.

This allows better representation of "visible" nodes across the tree (w.r.t the selected time slice), whilst preserving the desired map behavior.
@jameshadfield
Copy link
Member Author

This PR now extends the visibility array to allow nodes not to be displayed on the tree but to be processed by the map (as per proposal). I think this results in a much improved tree with no change to the map. I think this PR also effectively closes #473.

Before:
image

After:
image

@trvrb
Copy link
Member

trvrb commented Aug 31, 2018

This is a really fantastic change. I didn't realize how broken the tree visibility really was until you added the gray boxing. With this in place, the animation looks substantially better on the tree.

Definitely a hacky solution, but one that seems to work really well in practice. My only code issue is the use of 0, 1 and 2 for the visibility array. This seems unnecessarily opaque. Would it make sense to call this hidden, visibleMap (or visibleMapOnly) and visible (visibleMapTree) or some such? Keeps me from doing mental gymnastics while looking at code.

Copy link
Member

@rneher rneher left a comment

Choose a reason for hiding this comment

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

this is great. I have avoided showing animations so far because their behavior was so odd. regarding Trevor's proposal: having integers as states might be good for performance reasons (I doubt it makes a big difference though) but reduce readability.
What about defining states like HIDDEN=0, 'MAPONLY=1, VISIBLE=2` or similar and use these names in the code (akin to preprocessor directives in C)

@jameshadfield
Copy link
Member Author

Good ideas - now implemented using named constants NODE_NOT_VISIBLE, NODE_VISIBLE_TO_MAP_ONLY, NODE_VISIBLE.

@jameshadfield jameshadfield merged commit f2873af into master Aug 31, 2018
@jameshadfield jameshadfield deleted the temporal-visibility branch August 31, 2018 18:11
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.

3 participants