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

Fix download empty tree #1591

Merged
merged 3 commits into from
Nov 6, 2022
Merged

Fix download empty tree #1591

merged 3 commits into from
Nov 6, 2022

Conversation

joverlee521
Copy link
Contributor

@joverlee521 joverlee521 commented Nov 3, 2022

Description of proposed changes

Fixes the empty tree downloads when the tree is filtered.
This does not support the case of downloading multiple subtrees. I'm going to punt that for now and just added a note to the users that we do not support this feature. We can open another issue if this feature is ever requested.

I've made the treeToNewick function raise an error if it ever creates an empty tree since this seems like generally undesired behavior. I can drop this change if people think otherwise.

Related issue(s)

Resolves #1267

Testing

  • Checks pass

In cases where the user has filtered the tree, the `idxOfFilteredRoot`
is the root of the selected tips instead of `idxOfInViewRootNode`.
By using `idxOfFilteredRoot` as the root of the tree, this now acts the
same way as the manual work-around of clicking "Zoom to selected" then
downloading the tree.

However, this does _not_ fix the case where the filter may have created
multiple subtrees. If there are multiple subtrees without a most recent
common ancestor, this function will still produce an empty Newick tree.
Raise an error when `treeToNewick` creates an empty tree is created
because this is undesired behavior. Created a custom error class for
this so that we could include details in the warning notification for
the user.
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-fix-download-em-gc2rs3 November 3, 2022 00:18 Inactive
@joverlee521 joverlee521 requested a review from a team November 3, 2022 00:33
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

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

I'm not well-versed in the interactions here, but code looks ok to me by inspection and clicking around the review app to exercise the filtering + tree download seems to work as expected.

Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Thanks Jover! I agree that erroring in most situations like this is preferable -- newick trees can't represent the states that auspice can be in! We could probably work this out on download-modal rendering & not even allow downloading to be attempted, but the approach in this PR is just fine.

As an aside, the logic around zooming / inView nodes / visible nodes is getting pretty complex. In part this is because a lot of those features have been added over time and we haven't rethought the underlying architecture. Zooming is well overdue a rethink in auspice so hopefully we can address this then!

@jameshadfield jameshadfield merged commit 04c550d into master Nov 6, 2022
@jameshadfield jameshadfield deleted the fix-download-empty-tree branch November 6, 2022 23:19
@joverlee521
Copy link
Contributor Author

As an aside, the logic around zooming / inView nodes / visible nodes is getting pretty complex. In part this is because a lot of those features have been added over time and we haven't rethought the underlying architecture. Zooming is well overdue a rethink in auspice so hopefully we can address this then!

Definitely! Would love to revisit the xy zoom idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Download Data producing empty trees
4 participants