Size score-column precision to the column's variation, not its minimum - #97
matthewcornell wants to merge 3 commits into
Conversation
…ficant figures (signif) #88] Score columns took their decimal count from the smallest value in the column and applied it to every value via toFixed(), so a column's significant figures tracked its minimum rather than its variation. On the flusight dashboard the wis__log column spans [0.19, 1.27]; one decimal cleared the old rule's "nothing rounds to zero" bar, so 21 of 58 models rendered as the same "0.3" while wis in the next column carried four digits nobody could act on. Replace min_decimals_for_values() with score_decimals(), which picks the decimals that resolve the column's variation -- two effective digits in Ehrenberg's sense (1977, JRSS A 140(3), 277-297) -- and caps how many significant figures the column may carry. get_round_decimals() keeps its name, signature, and its relative-skill and coverage special cases, and delegates the rest. Both magnitude anchors are quantiles rather than extremes. The spread is the IQR, since the range is set by the worst model but the comparison that matters is among the contenders at the top. The cap is anchored on Q3 rather than on the maximum, which is the one deviation from the code in the issue thread: a max-anchored cap lets one blown-up submission set the digit count for everyone, flattening a column of values near 1.0 to "0", "1", "1" the moment a 52000 appears -- the same failure the IQR anchoring exists to prevent, reintroduced one line later. Per the discussion on #88 there is no minSigFigs floor: WIS and MAE land at 0 decimals ("184", "393"), which is what both reviewers preferred over "183.7". The rule stays decimals-based rather than significant-figure-based on purpose. The issue suggested d3's .3~r / .3g, i.e. R's signif(), but that rounds left of the decimal point -- a national-scale MAE of 12345.6 would render as 12300. toFixed() never does, and the cap only ever removes decimals, bottoming out at 0, so every integer digit survives at national scale. Add render_score() for the per-cell string. Values too small to survive the column's rounding render as "<0.01" (or ">-0.01") rather than as a bare "0", which is what keeps a column-wide rule from claiming a real non-zero score is zero. That string is also why the table's render callback now guards on the DataTables type: render() runs for every type including sort, and "<0.01" in a sort key would silently switch the column to lexicographic ordering. Display and filter get the string, everything else gets the number. Both hovertemplates now consume pre-formatted customdata instead of interpolating a .Nf spec, so a hover value and its table cell can never disagree about rounding. The colorbar tickformat is unchanged. One judgment call worth flagging for review: zeros are filtered out of the magnitude anchors but still count toward the IQR, so [0, 1.5, 2.3, 47.7] gives 1 decimal where [1.5, 2.3, 47.7] gives 2. A zero is a real score and belongs in the column's distribution. It is pinned by a test either way. Tests replace the min_decimals_for_values module wholesale, since it asserted the min-anchored contract directly. The new modules cover the 58-value flusight wis__log fixture (7 distinct rendered strings before, 34 after), the national-scale case, and both outlier directions. test/ui.js gains a cell-rendering module covering column values -> decimals -> cell string end to end plus the sort-key regression; its DataTables stub is now a shared stubDataTable() helper that captures the handed-over config. Bundles rebuilt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I've created a live preview here: http://hub-data-preview.s3-website-us-east-1.amazonaws.com/eval.html . You can compare it to production: https://reichlab.io/flusight-dashboard/eval.html |
|
I checked it out and it looks good to me. Thanks for tackling this, @matthewcornell ! |
|
Thanks, @nickreich ! @sbfnk : Any thoughts, or does this look good to you too? |
score_decimals() fell back to the full range whenever the IQR came out zero.
The comment justified that as covering "few rows, or many ties", but many ties
is exactly the case where the fallback hands the column back to the outliers
the IQR anchoring exists to keep out: [0.5 x8, 0.52, 52000] has a zero IQR, so
the range set the precision at 0 decimals and rendered the nine rows that
matter as "1" nine times over. That is the issue-88 failure, reintroduced one
fallback later -- the same shape as the max-anchored cap the parent commit
declined to take.
Widen the window in rungs instead: IQR, then P10-P90, then the full range. The
tied column above now resolves at 3 decimals ("0.500", "0.520"). The range is
still the last rung, but reaching it now means the column is tied from P10 to
P90, where there is nothing left to measure and the range is all that is left.
No-op on all four real flusight columns and on every existing test.
Document the one case the parent commit left implicit: maxSigFigs is applied as
a hard minimum against the spread, so it wins where the two disagree. Above
~1e4 that flattens neighbors the spread would have resolved ([12345.1, 12345.2]
wants 1 decimal and gets 0). That is the trade the cap exists to make -- the
loss is the sixth significant figure, not the first -- but it is worth naming
next to the maxSigFigs knob rather than leaving a reader to find it.
Narrow both hovertemplate comments. "A hover value and its table cell always
read the same" is not quite true: the table's decimals come from scores_table
(never disaggregated) and the plot's from scores_plot, so while disaggregation
is on the two panes can differ. State what the change actually buys instead --
hover now goes through the same rule as every other score display, including
render_score()'s '<0.01' floor and its '' for a missing score, where %{y:.Nf}
rendered a bare "NaN" (parseFloat() turns an absent score into NaN, not null).
Restore the covidhub tiny-WIS fixture dropped when the min_decimals_for_values
tests were replaced. It is the second real-data column on record, it lands at 5
decimals under the new rule, and all seven rows now render distinctly where the
old rule collapsed 0.000805 and 0.000759 onto "0.0008". Add a test pinning the
tie-heavy column in both directions. Bundles rebuilt.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I had Claude do a fresh analysis of this PR, and it found some issues. I had it address them (see 762d77f message for details) and updated the preview site. I think we're good to merge. |
| test('a tie-heavy column is not handed back to its outlier', assert => { | ||
| // the IQR is zero here, so the window widens to P10-P90 rather than dropping to the full | ||
| // range. Going straight to the range would let the 52000 set the precision and render the | ||
| // nine rows that matter as "1" nine times over - the issue-88 failure, one fallback later |
There was a problem hiding this comment.
Could this be more explicit about exactly what it is testing and what our stated desired behavior is?
I can't quite tell what the tests are asserting the desired behavior is. If we had the set of scores [0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.52, 52000] then I think we'd want 1 decimal place (maybe 2?). But I can't read from the code what it is testing.
There was a problem hiding this comment.
[Claude reply] Good call — that was hard to read, and I've pushed comments that state the case and the desired behavior for every test in get_round_decimals, score_decimals and render_score, plus a header over the score_decimals module laying out the rule as four numbered statements the individual tests refer back to.
On your example specifically: [0.5 ×8, 0.52, 52000] gives 3 decimals, not the 1–2 you expected, and I think 3 is right — but the disagreement is worth settling, since it's the rule itself and not a fallback misfiring.
Q1 and Q3 are both 0.5, so the IQR is zero and the window widens to P10–P90, giving a spread of 0.02. The rule then shows roughly two digits of that spread — 0.020 — which is 3 decimals. Two decimals is all it takes to merely tell 0.50 from 0.52; the third is the same two-effective-digits rule that gives the flusight wis__log column 2 decimals from its 0.161 spread. So the choice is between "enough to separate adjacent rows" and "two digits of the spread", applied consistently. The rows render as 0.500 and 0.520 (and the outlier as 52000.000, which is the ugly part).
If you'd rather target the first of those, that's a change to effDigits and not to the tests. The test comment now names the discrepancy in as many words so the next reader meets it where the surprise is.
nickreich
left a comment
There was a problem hiding this comment.
I'm approving, but ... I was trying to look over the test cases to assess how well the current code is capturing and dealing with the various scenarios that we laid out. I found it hard to read the code and understand this. It would help me if each test had a specific natural language statement of the situation that it is solving (the brief test('...') lines are not enough for me) and the desired behavior. I left one specific comment to this effect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@nickreich I had Claude add test comments. Please take a look. Thank you. |
Fixes #88.
Score columns took their decimal count from the smallest value in the column and applied it to every value via
toFixed(), so a column's significant figures tracked its minimum rather than its variation. On the flusight dashboardwis__logspans[0.19, 1.27], so one decimal cleared the old rule's "nothing rounds to zero" bar and 21 of 58 models rendered as the same0.3— whilewisnext to it carried four digits nobody could act on.What changed
min_decimals_for_values()→score_decimals(): picks the decimals that resolve the column's variation (two effective digits, Ehrenberg 1977) and caps how many significant figures the column may carry.get_round_decimals()keeps its name, signature, and its relative-skill / coverage special cases, and delegates the rest.New
render_score()for the per-cell string. Values too small to survive the column's rounding render as<0.01/>-0.01rather than a bare0, so a column-wide rule never claims a real non-zero score is zero.Effect on the real 58-model flusight data:
wis__log0.19 0.42 0.26ae_median__log0.32 0.68 0.39wis184 67 53 393ae_median291 101 77 498Distinct rendered strings in
WIS (log): 7 → 34 across 58 rows.Two decisions from the #88 thread, for review
No
minSigFigsfloor.WISandMAEland at 0 decimals —7.65 → "8",183.7 → "184". @sbfnk: a short value isn't really one sig fig when the column's shared decimal position supplies the place value. @nickreich: fine dropping the decimal place on whole-number-scale columns. The floor considered earlier in the thread would have forced183.70, so it's gone.The cap is anchored on Q3, not
Math.max— the one deviation from the code in my issue comment. A max-anchored cap lets one blown-up submission set the digit count for everyone:That is the same failure the IQR anchoring exists to prevent, reintroduced one line later. It's a no-op on all four real columns.
Not
signif()The issue suggested d3's
.3~r/.3g, i.e. R'ssignif()— but that rounds left of the decimal point, and a national-scale MAE of12345.6would render as12300. This rule stays decimals-based:toFixed()never touches integer digits, and the cap only ever removes decimals, bottoming out at 0. Every integer digit survives at national scale. Pinned by a test.Also worth a look
rendercallback now guards on the DataTablestype.render()runs for every type includingsort, and a<0.01sort key would silently switch the column to lexicographic ordering. Display and filter get the string; everything else gets the number.customdatainstead of interpolating a.Nfspec, so a hover value and its table cell can't disagree about rounding. Colorbartickformatunchanged.[0, 1.5, 2.3, 47.7]gives 1 decimal where[1.5, 2.3, 47.7]gives 2. My reasoning is that a zero is a real score and belongs in the column's distribution. Pinned by a test either way — easy to flip if you disagree.Testing
npm test— 82 pass, 0 fail. Themin_decimals_for_valuestests are replaced wholesale (they asserted the min-anchored contract directly). New coverage: the 58-value flusightwis__logfixture, the national-scale case, both outlier directions, and atest/ui.jsmodule running column values → decimals → cell string end to end plus the sort-key regression. Bundles rebuilt.🤖 Generated with Claude Code