-
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
Conversation
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.
Looks good, just a few tiny comments. Tested downloads on the review app which seem to be working.
I didn't go as far as validating the downloaded data was accurate, although I was confused why downloading in AA mode for dengue/all seemed to be missing a few codons. This seems like if anything it's probably related to that data and not to this PR:
gene position entropy
2K 5 0.007
2K 8 0.925
2K 11 0.380
2K 12 0.303
2K 13 0.925
export const entropyTSV = (dispatch, filePrefix, entropy, mutType) => { | ||
const lines = mutType === "nuc" ? ["base"] : ["gene\tposition"]; | ||
lines[0] += entropy.showCounts ? "\tevents" : "\tentropy"; | ||
entropy.bars.forEach((bar) => { |
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.
eharkins is talking about the fraction of the genome that the zoom currently shows
Yes.
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...
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.
the fraction of the genome that the zoom currently shows
I didn't take this into account. We could, if desired.
diversity among currently zoomed/selected tips in the tree
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.
We frequently get requests to share the data displayed in the diversity panel. Here we address this by allowing download of the data behind the graph. This is the simplest implementation, as to download all data together ({events,entropy}U{nt,aa}) would require recalculations within the download logic as these data are not stored by auspice when not being displayed.
bb5094b
to
28f6a44
Compare
No -- this is a good point to raise. We only report values for variable positions, which is why there are "missing" codons. I'll make sure to add this to the docs when I write them. |
We frequently get requests to share the data displayed in the
diversity panel. Here we address this by allowing download of
the data behind the graph. This is the simplest implementation,
as to download all data together ({events,entropy}U{nt,aa})
would require recalculations within the download logic as these
data are not stored by auspice when not being displayed.
closes #1143