-
Notifications
You must be signed in to change notification settings - Fork 166
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
Allow displayed diversity data to be downloaded #1144
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@jameshadfield if the entropy view is "zoomed" aka only showing a subset of sites, do we want to limit the download to just those sites? Defaulting to all of them seems ok to me since getting more data that you want is the worst case, but just wanted to check.
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.
My guess is that downloading the data shown on the screen (filtered by currently visible tips) is the better / more powerful behavior. Just need to be clear to the user that this is what's going on. It matches the way that NEWICK download works as well.
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 think the newick download is always the entire tree. but we have an option to download only the metadata for the displayed tips.
There are two different zooms that are relevant ehre. I think @eharkins is talking about the fraction of the genome that the zoom currently shows, while @trvrb talks about diversity among currently zoomed/selected tips in the tree (correct me if I am wrong). In the former case, more data doesn't hurt. In the latter case the zoom/filter defines what diversity is. so here providing the current state is most sensible but it needs to be obvious IMHO...
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.
Yes.
👍
The current state of this is similar to how it is done for downloading other data that is filtered according to visible tree trips:

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 didn't take this into account. We could, if desired.
This is taken into account, as it defines the underlying diversity data. I dynamically change the message printed in the download modal to indicate this. For instance, changing to nucleotide, events, and zoomed to a sub-tree will display "The data behind the diversity panel showing a count of changes across the tree per nucleotide. Restricted to strains which are currently displayed (n = 134/543)."
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.
Yes. I was confused. I was thinking of the "selected metadata". I would just export the entire genome, rather than the in view portion.