-
Notifications
You must be signed in to change notification settings - Fork 163
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
Color tree by measurements #1924
Conversation
Since our attributes are all strings due to the use of `Object.keys()`, this did not match the legend values when they were numeric and resulted in the ordering of mean and raw values to be arbitrary. This has been a bug all along, but I didn't notice it until I was working on the coloring by measurements feature that focused on continuous numeric legend values. This commit converts all legend values to strings in the Redux selector so that ordering works across all color scales.
Add basic feature where clicking on the grouping value in the measurements panel will color the tree tips by their average measurement values. I decided to use the existing actions for adding metadata/coloring and changing the color by so that the new measurements will work as expected of normal colorings. Many improvements on the feature to come in subsequent commits.
Previously, we were only using the `ADD_EXTRA_METADATA` action for drag-n-drop of metadata, which is hard-coded to `categorical`.¹ Now, we are using the same action to add measurement data for coloring and we want to exclude the `continuous` data from being used as filters. ¹ <https://github.com/nextstrain/auspice/blob/8a6e56fa4b926de7230bf3bd35fa9e590d878fd5/src/actions/filesDropped/metadata.js#L80>
Using the default continuous color range to build the color scale.¹ Use quantile of all measurements as anchors for the continuous measurements color scale to get an even distribution of colors across the color range. In the future, we might want to expose color scale as collection display parameters so that collections can specify their own colors. ¹ <https://github.com/nextstrain/auspice/blob/c0ca42fed852bcd2709dfa853e134cd7ca6f2f46/src/util/colorScale.ts#L283>
Doing this in preparation for using the same functions to get the filter measurements within `applyMeasurementsColorBy` in the following commit.
Only include measurements for calculating the average value per strain that pass the measurements filters. This allows users to see coloring on the tree for a subset of measurements, e.g. from single source.
If the current `colorBy` state changes, then we lose the value of the selected grouping for measurements colorings. Save this as a separate control state so that it can be used to remove stale measurements metadata and colorings. This state is also used in the measurements panel to prevent extra dispatches if the user clicks on the same grouping value multiple times.
When applying a new measurement coloring, remove the old measurement metadata and coloring from the state before adding the new data to ensure that we only display the new coloring data. This adds a new `REMOVE_METADATA` action type that is added to all the reducers that update state for the `ADD_EXTRA_METADATA` action.
Using `batch` to ensure the multiple dispatch actions only result in one re-render in React.¹ This is also _required_ to prevent error in calcColorScale during extra renders: 1. REMOVE_METADATA removes current measurements coloring from metadata.colorings 2. This triggers the componentDidUpdate in controls/color-by, which dispatches changeColorBy. 3. calcColorScale throws error because the current coloring is no longer valid as it was removed by REMOVE_METADATA in step 1. ¹ <https://react-redux.js.org/api/batch>
Doing this in preparation for using the same code to update measurement metadata and color scale when there are changes to the displayed measurement data, i.e. different collection, group-by, and filtering, without changing the color-by.
Remove the measurement coloring if it's no longer valid for the new collection. If the coloring is still valid, then update the metadata and coloring to match the new measurements. The `updateMeasurementsColorData` is a separate function because I will be using the same logic in subsequent commits when changing group by or filters.
Remove the measurement coloring if it's no longer valid for the new group by. If the coloring is still valid, then update the metadata and coloring to match the new group of measurements.
Although measurements filters do not affect the measurements color grouping value, the measurements metadata needs to be updated to reflect the filtered measurements.
Reflect the string or number type allowed in the Auspice JSON.¹ Doing this in preparation for using the same type for measurements coloring info. ¹ <https://github.com/nextstrain/augur/blob/77ae31ef3374da04aae7f702c123072a58b30648/augur/data/schema-auspice-config-v2.json#L46>
Doing this in preparation for using the same code to handle coloring URL params for measurements color-by. Includes new types to simplify the function return type.
Add the needed measurements metadata and coloring during `createStateFromQueryOrJSONs` so that the measurements coloring can be handed as a normal coloring with the URL param `c=m-<grouping-value>`.
When the coloring is the measurements coloring, add a crosshair in the measurements panel's y-axis to indicate the grouping value used for the coloring.
Allow the user to click on the same measurements grouping to change coloring if the current colorby is _not_ a measurements coloring.
This only adds the crosshair on the tip during initial render of the tree, i.e. if the measurements coloring is in the URL. Subsequent commits will add/remove crosshair as needed from interactions within the app.
Re-draw the measurements crosshair when modifying SVG so that the crosshair position can stay in-sync with the corresponding tip. A future improvement would be to update the crosshair's x and y instead of removing and re-drawing.
Ensures that the measurements coloring crosshair gets updated when we change to a different coloring. Removes the crosshair if the coloring is _not_ a measurements coloring.
When changing slides, the tree colorings need to change even if the colorBy stays the same as long as the measurements coloring data changes.
When changing slides, the old measurements coloring data need to be removed before the new measurements coloring data get added to ensure that they aren't mixed in the tree of the new slide.
@@ -466,7 +470,7 @@ export const addColorByAttrToGroupingLabel = (ref, treeStrainColors) => { | |||
svg.selectAll(`.${classes.yAxis}`).select(".tick") | |||
.each((_, i, elements) => { | |||
const groupingLabel = select(elements[i]); | |||
const groupingValue = groupingLabel.text(); | |||
const groupingValue = groupingLabel.select(`.${classes.groupingValue}`).text(); | |||
const groupingValueColorBy = treeStrainColors[groupingValue]; |
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 that value reflects the color bin that the other reference viruses map to based on their measurements against the selected reference and not some summary statistic of all measurements for the other references. Is that right?
Yes, this is reflecting the color of the strain in the tree
const sortedValues = collection.measurements | ||
.map((m) => m.value) | ||
.sort((a, b) => a - b); | ||
|
||
// Matching the default coloring for continuous scales | ||
const colorRange = colors[9]; | ||
const step = 1 / (colorRange.length - 1); | ||
const measurementsColorScale: [number, string][] = colorRange.map((color, i) => { | ||
return [quantile(sortedValues, (step * i)), color] | ||
}); |
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.
Are the bins for the color ramp for the measurements scale fixed across all measurements in a given collection?
Yes, the color scale is created using all measurements in a given collection. I did this to have a stable coloring scale for each collection.
Suggested by @genehack in review <#1924 (comment)>
Suggested by @huddlej in review <#1924 (review)>
If the current color by is a measurement grouping, clicking on the same grouping will toggle back to the previous non-measurements coloring. Suggested by @huddlej in review <#1924 (review)>
Reflect reality where scale is not required in the JSON schema <https://github.com/nextstrain/augur/blob/19308f0a5a393c8f9b6585eb34f4aeb0a99dac93/augur/data/schema-export-v2.json#L102> Prompted by subsequent commit to add a boolean type coloring that does not have a custom scale.
Adds a new attr and boolean type coloring `_hasMeasurementColor` to the tree so that it can be used to filter to only tips that are colored by the measurements coloring.
Automatically filter to the colored tips when applying the measurements coloring by clicking on the grouping in the measurements panel. The filtering is _not_ applied when changing coloring via dropdown. This commit also ensures that 1. tree visibility is updated when measurements coloring data changes 2. the measurements coloring filter is removed when no longer valid
I plan to merge this next |
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 @joverlee521. Read through the code and played around a little with a measurements dataset I had, although I didn't test out the narratives or change collections behaviour. I don't think any of my comments are blocking.
// Convert legendValues to string to ensure that subsequent attribute matches work as intended | ||
const legendValues = useSelector((state: RootState) => state.controls.colorScale.legendValues.map((v): string => v.toString()), isEqual); |
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 few minor points - don't change them if you don't agree with them:
Since our attributes are all strings due to the use of
Object.keys()
,
this did not match the legend values when they were numeric and resulted
in the ordering of mean and raw values to be arbitrary. This has
been a bug all along, but I didn't notice it until I was working on the
coloring by measurements feature that focused on continuous numeric
legend values.
This commit converts all legend values to strings in the Redux
selector so that ordering works across all color scales.
toString
will raise an exception on certain values (null
,undefined
etc) whereasString
will not, and the latter can be succinctly written as.map(String)
. ThelegendValues
areany[]
so these values may occur? I'm surprisedtsc
didn't complain.- Not being super familiar with measurements terminology, I didn't understand "attributes" in the commit message and had to dive into the code. I think it's node attributes / node values, right? Where is the
Object.keys
you mention - I followed the code a little bit but didn't find it.
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.
Ah good point about toString
raising exceptions, I'll update to String
. (Edit: updated in 921161c.)
Not being super familiar with measurements terminology, I didn't understand "attributes" in the commit message and had to dive into the code. I think it's node attributes / node values, right?
These are referring to the coloring attributes, so yes these correspond to the node attributes / node values.
Where is the Object.keys you mention - I followed the code a little bit but didn't find it.
These are referring to where the node attributes get compared to the legend values for ordering. These are in measurementsD3.js, within jitterRawMeansByColorBy and drawMeansForColorBy.
@@ -65,10 +65,18 @@ interface Query extends MeasurementsQuery { | |||
[key: string]: string | string[] | |||
} | |||
|
|||
|
|||
const hasMeasurementColorAttr = "_hasMeasurementColor"; |
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.
Probably beyond this PR, but this makes me think about some improvements to the filter badges - they haven't seen any development since they were first introduced and filtering has become a much bigger part of workflows since then.
It'd be nice to show the title not the key when hovering, and even nicer if we could somehow render this in the badge itself, as a badge simply showing "true" isn't very intuitive. We already do both of these in the sidebar
Would this (the _hasMeasurementsColor
filter) be better as part of the separate measurements filtering section?
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.
It'd be nice to show the title not the key when hovering, and even nicer if we could somehow render this in the badge itself, as a badge simply showing "true" isn't very intuitive.
Agree, these would be nice, but it might overcrowd the badges with the longer titles. We can revisit the design of the filter badges separately.
Would this (the _hasMeasurementsColor filter) be better as part of the separate measurements filtering section?
I think it makes sense to keep the _hasMeasurementsColor
filter in the overall filters since it is filtering the tree node attributes. The measurements filtering section is specific to filtering measurements records in the measurements panel.
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 it makes sense to keep the _hasMeasurementsColor filter in the overall filters since it is filtering the tree node attributes. The measurements filtering section is specific to filtering measurements records in the measurements panel.
Ahh, this is clarifying. Thanks!
* 2. This triggers the componentDidUpdate in controls/color-by, which dispatches changeColorBy. | ||
* 3. calcColorScale throws error because the current coloring is no longer valid as it was removed by REMOVE_METADATA in step 1. | ||
*/ | ||
batch(() => { |
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 know about batch
- was a good opportunity to catch up with some of the many changes in the redux/react world. It seems that React 18 will do this automatically (react 18 upgrade PR: #1659). I wondered why you didn't just add the new node info, change colour, then delete the old data... however since the node.attrs
key names are reused I can imagine this causes some undesirable side-effects?
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 wondered why you didn't just add the new node info, change colour, then delete the old data... however since the node.attrs key names are reused I can imagine this causes some undesirable side-effects?
Yeah, things can get complicated if the node.attrs key names are the same but the underlying values changed. A concrete example is changing measurements collections. They might have the same reference strain (which means they have the same node.attrs key), but completely different measurements so the data needs to be completely updated. It's cleaner to remove old data then apply the new data instead of trying to keep track of which nodes needs to be updated.
src/util/treeMiscHelpers.js
Outdated
if(attrName in node.node_attrs){ | ||
delete node.node_attrs[attrName]; | ||
} |
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.
You can drop the conditional - delete
doesn't throw if the attrName
isn't a property
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.
Oops, Python bleeding into JavaScript...dropped conditional in aead1cf.
* 2. This triggers the componentDidUpdate in controls/color-by, which dispatches changeColorBy. | ||
* 3. calcColorScale throws error because the current coloring is no longer valid as it was removed by REMOVE_METADATA in step 1. | ||
*/ | ||
batch(() => { |
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 don't think it's worth changing what you've done (which includes a good docstring) but an alternative approach to consider for patterns like this if we use them again is to make "remove metadata" a thunk which checks that the attr being removed isn't currently used as a coloring, and if it is then thee thunk can do something about it.
// Update treeToo if exists | ||
if (treeToo && treeToo.loaded) { | ||
addNodeAttrs(treeToo.nodes, newColoringData.nodeAttrs); | ||
} |
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.
Can we have two trees + measurements? I've come to the opinion that we should drop a lot of functionality when viewing two trees, so that if the second tree is displayed we don't allow measurements, frequencies, map to be displayed.
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.
Two trees + measurements is definitely possible right now, where the measurements panel always uses the measurements JSON from the main tree.
I'm not entirely sure how useful it is though...could it be useful for HA:NA + measurements?
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 not entirely sure how useful it is though...could it be useful for HA:NA + measurements?
I can't answer that, but we should find the answer. Two trees complicates Auspice quite a lot, and I think it would be a worthwhile push to limit the cases in order to simplify the code.
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.
We can revisit simplifying code in the future, starting conversation in Slack.
const handleClickOnGrouping = useCallback((grouping: string): void => { | ||
if (grouping !== colorGrouping || !isMeasurementColorBy(colorBy)) { | ||
dispatch(applyMeasurementsColorBy(grouping)); | ||
} else if (grouping === colorGrouping && isMeasurementColorBy(colorBy)) { | ||
// Clicking on the same grouping twice will toggle back to the previous non-measurements coloring | ||
dispatch(changeColorBy(prevNonMeasurementColorBy.current)); |
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.
[Comment "Click on measurements grouping to toggle back to previous coloring"]
This'll leave the _hasMeasurementsColor
filter enabled. Just a heads up - not a comment on whether that's the desired behaviour or not.
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.
Thanks for bringing this up! I left this in as a "feature", but not sure if others noticed.
I thought it'd be useful to see the filtered tips in the other coloring option as you quickly toggle back and forth by clicking on the measurements grouping. I'm leaving it as-is unless I hear otherwise in the future.
Switching to `String` to avoid potential exceptions raised by `null` or `undefined` values in the `legendValues` as suggested by @jameshadfield in review <#1924 (comment)>
Suggested by @jameshadfield in review <#1924 (comment)>
Awesome to have this merged! This is a very cool feature. A couple post-merge points / desires (based on what's currently at https://dev.nextstrain.org/groups/nextflu-private/flu/seasonal/2024-12-13/h1n1pdm/2y/titers/ha).
The box in the side bar is properly clear:
|
Ah, I didn't include it in the hover over each row because you have to click on the grouping in the y-axis. There is a small pop-up when you hover over group in the y-axis, although seems like it takes a brief moment for it to pop up: Screen.Recording.2025-01-22.at.3.07.15.PM.mov |
Description of proposed changes
Clicking on a measurements group in the y-axis of the measurements panel will add the new coloring to the tree and a new color by to the drop-down. A crosshair icon is added to annotate the coloring group in the measurements panel and in the tree if there is a tip with a name that matches the coloring group. The measurements coloring uses the existing URL param for coloring, formatted as
c=m-<grouping-value>
.The coloring on the tree represents an average of the measurement values for the matching test strain within the selected measurements group. Filtering the measurements data (e.g. by source) will also filter the measurements values used to calculate the average value. Note that if the measurements filter removes all records within the selected color grouping, then the tree will be all grey. If the measurements coloring is no longer valid when changing the measurements collection or group by, then the tree will revert to its default coloring with a warning notification.
Testing
Related issue(s)
Resolves #1819
Checklist