-
Notifications
You must be signed in to change notification settings - Fork 161
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
Extend usefulness of tip labels #1668
Conversation
You can test the above dataset using this PR at https://nextstrain-s-nextstrain-wpreme.herokuapp.com/staging/monkeypox/mpxv/auspice-pr-1668 |
There's conflicts with files I've edited since this PR was opened. Addressing them with a rebase onto latest |
56d531e
to
b837f36
Compare
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 played around on the review app. Overall nice improvements! Just a few minor things.
Any normal `node_attr` in the tree can be used as a tip label, as well as the special-cases of strain-name and "none". Previously we only allowed valid colorings. Normal here means a node_attr value which is an object with a "value" key, thus excluding special-cases such as "div", "hidden", "vaccine" etc - see the JSON schema for more context <https://github.com/nextstrain/augur/blob/master/augur/data/schema-export-v2.json>
b837f36
to
962a2e2
Compare
This is a companion commit to changes in Auspice <nextstrain/auspice#1668> which allow any node-attr to be used as the tip label and which displays this tip label in the node hover/click boxes as well as alongside nodes (at suitable zoom levels). This allows us to drop the `set_final_strain_name` script and ensures all node names are unique which improves the behaviour of Auspice This commit only changes the Nextstrain datasets, not the Nextclade ones
I've rebased this onto master and also simplified the process by allowing any mpox testing datasets available at:
or use this test version of auspice.us @j23414 / @kimandrews if you have any applicable datasets could you test them here? |
Testing <nextstrain/auspice#1668> Following changes made in mpox <nextstrain/mpox@4a268ba>
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.
Changes make sense to me.
Used nextstrain/zika#72 to produce testing zika dataset for the new Auspice view.
for (const [attrKey, attrValue] of Object.entries(d.node_attrs || {})) { | ||
if (typeof attrValue === 'object' && 'value' in attrValue) { | ||
nodeAttrKeys.add(attrKey) | ||
} | ||
} |
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.
non-blocking, probably more of a change in Augur
I was looking at the augur export schema to figure out which attrs this would be included here. Worth noting that we would not be able to use accession
as the tip label because it does not use the value
key.
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.
Yeah, that's a good point. I wish we had made the schema so that every node-attr was the same structure.
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'm going to refrain from addressing this here, but I do think moving all node_attrs
values to objects will reduce complexity in Auspice a lot, see for example all these different getters. We don't have to do this on the augur side (necessitating schema changes and all that), we could probably do it within treeJsonProcessing.js.
@@ -56,10 +62,7 @@ export function collectAvailableTipLabelOptions(colorings) { | |||
*/ | |||
{value: 'none', label: "none"}, | |||
{value: strainSymbol, label: "Sample Name"}, |
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.
One oddity I noticed here is the hardcoding of node.name
to "Sample Name" which is a little jarring when we use accessions for node.name
.
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.
A couple small nits and a bug
This generally better reflects the intention of changing the tip label key, which was previously only reflected in text drawn to the RHS of tree nodes at certain zoom levels. The motivating use case is the proliferation of datasets using an accession for `node.name` but who wish to see node names of another piece of metadata. This is possible by adding the desired name as a node_attr and (optionally) setting this as the default tip label. Note that the order of items in the node-clicked modal has been slightly altered and is now more consistent across internal branches, terminal branches & tips
962a2e2
to
399c501
Compare
The motivating use case is the proliferation of datasets using an accession for
node.name
but who wish to see node names of another piece of metadata. See commit messages for more details.Using our current monkeypox/mpxv dataset with the following differences:
node.name
is the accession (which is the uniq ID used throughout the pipeline). This removes the need for the pipelines to use JSON-modification scripts such asset_final_strain_name.py
.node_attrs
reflecting the original strain name. (Any name can be used here.)"tip_label": "strain"
added as adisplay_defaults
On hover boxes show both the chosen tip-label and the underlying node name (accession). (On-click modals are similar)
Filtering to duplicate strain names works intuitively