Fix bounds matching for zero values #1643
Conversation
This fixes a common bug for continuous scales where nodes have (valid) zero values. The bound for the first (smallest) legend entry would have zero as it's lower value, and as we strictly matched `v \in [a,b)` the node was never included as a match. This presented in two places: 1. Hovering over a legend swatch would not highlight (increase the radius of) matching nodes whose value was zero 2. The frequencies stream graph would map zeros to "unassigned" rather than their correct legend bin. There were a few code changes needed to case (2) as we also had falsey checks (and 0 is falsey). Note that the algorithm to create legend bounds is problematic and should be rewritten. This bug was first identified by Hugh Haddox, but others may have noticed it over the years?!? The /flu/seasonal/h3n2/ha/2y dataset is useful to test this bug: * `c=ne_star` - most of the tree has zeros, none are unassigned * `c=projected_frequency` - both unassigned and zero tips. * `c=fitness` failure to match lowest legend entry due to the bug in the legend bounds creation
jameshadfield
force-pushed
the
legend-bound-matching
branch
from
1983f07 to
c0baa80
Compare
|
Top image is auspice 2.45.0 at https://nextstrain.org/flu/seasonal/h3n2/ha/2y?c=ne_star&d=tree,frequencies&p=full. |
There was a problem hiding this comment.
Nice catch, @Haddox, and thanks for the fix, @jameshadfield! I can't speak to the code as much, but from the user experience side I have noticed the legend on-hover issue previously and assumed it was a "feature". I never noticed the frequencies panel issue with the "unassigned" label for continuous traits, since I rarely look at that kind of view in my builds (usually its frequencies of categorical labels when "unassigned" can be common and intentional).
Comparing the review app to the production app, the behavior now seems to be what I would expect.
|
Great catch! I had also noticed the lack of highlighting on hover for the first item in a continuous trait - but thought it may be an error on my side in making the dataset and wasn't bothered enough to create an issue (I know, it's weird as I usually open issues for everything...) |
|
Actually - the same bug seems to happen for minima in general, not just for 0? When you hover over -3.62, it doesn't highlight the blue dot - the blue dot is never highlighted -> same bug?
https://next.nextstrain.org/nextclade/sars-cov-2/21L?c=ace2_binding |
That's #1644 -- this PR fixed the bugs around comparing node values to the specified minima, and that issue describes a related bug where the minima is always set as |
This fixes a common bug for continuous scales where nodes have (valid) zero values. The bound for the first (smallest) legend entry would have zero as it's lower value, and as we strictly matched
v \in [a,b)the node was never included as a match. This presented in two places:There were a few code changes needed to case (2) as we also had falsey checks (and 0 is falsey).
Note that the algorithm to create legend bounds is problematic - I'll make a separate issue about this.
This bug was first identified by Hugh Haddox, but others may have noticed it over the years?!?
The /flu/seasonal/h3n2/ha/2y dataset is useful to test this bug:
c=ne_star- most of the tree has zeros, none are unassignedc=projected_frequency- both unassigned and zero tips.c=fitnessfailure to match lowest legend entry due to the bug in the legend bounds creation