Limit x-axis by filtered measurements #1827
Conversation
Replace the clunky use of `useMemo` with built in `useCallback`. Based on React docs, this should be completely equivalent <https://react.dev/reference/react/useMemo#memoizing-a-function>
Resolves #1814 Includes a new `useDeepCompareCallback` wrapper to be able to do a deep comparison of the filtered measurements array so that the `xScale` does not change on every re-render.
Add padding to x-axis by padding the domain of the xScale. Copied the example shared in open issue to support this natively in d3-scale <d3/d3-scale#150 (comment)>
nextstrain-bot
temporarily deployed
to
auspice-measurements-x--bzidhe
There was a problem hiding this comment.
This is awesome, @joverlee521! I actually like how the axis scale doesn't change between the raw and mean/std displays, since it allows me to toggle between those displays without the panel context jumping around.
Playing around with the paper dataset, this view shows the expected domain on the x-axis for the two references:
When I dropped the first reference, the x-axis domain didn't update which seemed like a bug here.
Then I switched to the raw display and saw that there actually are measurements at the far left of the domain that are plotted in gray as an "undefined" clade and that don't appear in the mean/std view:
This seems like a bug with the dataset (that it doesn't define a clade for every reference) and a separate bug with the measurements panel (that it doesn't show a mean/std anyway even when the clade is "undefined"). That's not a bug for this PR, though, (it happens in the production app) and the way you've implemented a fixed x-axis domain for both raw and mean/std views actually reveals the underlying data issue which is great.
I only noticed a couple of edge case issues where the std dev could exceed the lower and upper bounds of the x scale. For example, in the preview app if I filtered the source to "nimr-sep-2011-2011-06-24" and the clade reference to "158N/189K", I get the following mean/std view:
And this is the raw view:
That's a pretty extreme case, though, so maybe it's ok for now?
| return ( | ||
| scaleLinear() | ||
| .domain(extent(measurements, (m) => m.value)) | ||
| .domain(padLinear(extent(measurements, (m) => m.value), 0.1)) |
There was a problem hiding this comment.
This proportion of 0.1 is the kind of parameter I could imagine exposing to users some day through the measurements config JSON, since there could be dataset-specific reasons to prefer a lower or higher value. Maybe for the sake of remembering what this value means here and eventually exposing it later, could we define 0.1 as a variable in this code?
There was a problem hiding this comment.
Yeah, that's totally fair! I'll add it as an optional param with a default value for now and we can feed in a user defined value in the future if desired.
There was a problem hiding this comment.
Updated in c302939
Based on feedback from @huddlej <87b4446#r1720389378> I added as optional param here with a default value of 0.1 In the future, we can potentially feed in a user defined value from the measurements JSON.
Fixes bug flagged by @huddlej in review <#1827 (review)> Previously, measurements that were group by `undefined` did not have a mean for display because the panel was only iterating over existing legend values.
Ah, this is a case where the test strain does not have a defined clade. Looks like they don't get included in the mean display because the panel was adding means for the legend values only. This should be fixed with b201e5c.
Huh, I would think this is an issue outside of this PR if those were the only two data points the measurements collection. I'll leave it as-is for now since it's such an extreme case. |
Tracking in #1828 as a future TODO. If there's no other feedback, I'll merge this tomorrow. |
|
Awesome! Thank you again, @joverlee521! |
preview of measurements panel paper dataset
Description of proposed changes
Resolves #1814
Raw measurements
Before:
After:
Mean ± SD
It's not perfect for the mean ± SD view because the xScale is based on the underlying raw data.
Before:
After:
Checklist