measurements: order means by legend values #1566
Conversation
nextstrain-bot
temporarily deployed
to
auspice-measurements-im-cja7xm
| // Order the color groups by the legend value order so that we have a stable order for the means | ||
| orderBy( | ||
| Object.entries(colorByGroups), | ||
| ([attribute]) => legendValues.indexOf(attribute), |
There was a problem hiding this comment.
This doesn't work when the legendValues are numerics (legendValues.indexOf(attribute)=-1) and thus the ordering isn't working for continuous colour scales (date, antigenic advance etc).
The legendValues are inherently ordered, so could we not simply replace the orderBy call with the following?
legendValues.filter((v) => String(v) in colorByGroups)
.forEach((attribute) => {
const {color, values} = colorByGroups[attribute];
...This same incantation is used in one other place in auspice, so might be worth reviewing that as well (I haven't done so):
auspice/src/components/measurements/index.js
Line 176 in 2eb4bb3
There was a problem hiding this comment.
Ah, thank you for catching that! I'll update to your suggestion. That's much easier to read and understand
(Updated in b445e43)
There was a problem hiding this comment.
In the other use of orderBy that you pointed out, the fieldOrder is not guaranteed to include every single display field. We want to order displayFields by the fieldOrder first then the remaining fields can fall into their existing order.
There was a problem hiding this comment.
In the other use of orderBy that you pointed out, the fieldOrder is not guaranteed to include every single display field. We want to order displayFields by the fieldOrder first then the remaining fields can fall into their existing order.
👍 Sounds good. And since displayFields is a list of keys, they won't be numeric (which is the underlying potential bug here).
| @@ -305,7 +305,12 @@ export const drawMeansForColorBy = (ref, svgData, treeStrainColors) => { | |||
| // 2 x subplotPadding for padding around the overall mean display | |||
| const ySpacing = (layout.subplotHeight - 4 * layout.subplotPadding) / (numberOfColorByAttributes - 1); | |||
| let yValue = layout.subplotPadding; | |||
| Object.entries(colorByGroups).forEach(([attribute, {color, values}]) => { | |||
| // Order the color groups by the legend value order so that we have a stable order for the means | |||
| orderBy( | |||
There was a problem hiding this comment.
I'm not against lodash per-se (we use it a little bit in lots of places in auspice, and orderBy is used elsewhere in the measurements panel) but each time I feel there's a trade-off between learning another function vs writing out the slightly longer JS code. This trade-off varies across different lodash functions. Here I think a sort call would be simpler (at least, for others reading the code), but I'm interested in your general thoughts here? (It's not a request to change, especially as we're already importing it elsewhere).
There was a problem hiding this comment.
lodash is my personal preference because I trust it to do the right thing more than my own version of a sort call. I think that has definitely made me over-reliant on lodash and then miss the simpler ways of doing things like your suggestion above.
| @@ -305,7 +305,12 @@ export const drawMeansForColorBy = (ref, svgData, treeStrainColors) => { | |||
| // 2 x subplotPadding for padding around the overall mean display | |||
| const ySpacing = (layout.subplotHeight - 4 * layout.subplotPadding) / (numberOfColorByAttributes - 1); | |||
| let yValue = layout.subplotPadding; | |||
| Object.entries(colorByGroups).forEach(([attribute, {color, values}]) => { | |||
| // Order the color groups by the legend value order so that we have a stable order for the means | |||
There was a problem hiding this comment.
Did we ever consider doing this for the raw dots?
There was a problem hiding this comment.
Yup, we've discussed ordering the raw dots by color before (it's part of #1463).
I think that will be a bigger change because of the jitter. Currently, the jitter is calculated during loading of the sidecar JSON, so will need to think through how to rework that...
There was a problem hiding this comment.
I think that will be a bigger change because of the jitter. Currently, the jitter is calculated during loading of the sidecar JSON, so will need to think through how to rework that...
Thanks for the reference. I agree it's not for this PR, it just became apparent while reviewing it 😄
joverlee521
force-pushed
the
measurements-improvements
branch
from
90623cc to
b445e43
Compare
nextstrain-bot
temporarily deployed
to
auspice-measurements-im-cja7xm
Keep the color-by means in a stable order so that order does not change when applying different filters to the data. This makes it easier to do comparisons across groups and across different filters. This is the easier route for stable ordering, but in the future we can consider ordering by display order of groups in the tree.
joverlee521
force-pushed
the
measurements-improvements
branch
from
b445e43 to
9a03e6b
Compare
nextstrain-bot
temporarily deployed
to
auspice-measurements-im-cja7xm
Keep the color-by means in a stable order so that order does not change when applying different filters to the data. This makes it easier to do comparisons across groups and across different filters.
This is the easier route for stable ordering, but in the future we can consider ordering by display order of groups in the tree.