refactor(metrics): unify the percentile convention; address #417 round-3 review

[nvzhihanj]

Follow-up to #417 (early-stopping percentile estimates), two commits:

1. 06f3b2b — address the post-merge round-3 review of #417

All eight findings from the round-3 review council on #417 (each verified against HEAD before fixing):

• high / data-integrity: the feat(metrics): opt-in MLPerf early-stopping percentile estimates (LoadGen parity) #417 empty-series preservation was dead code on the real consumer path — Report.from_snapshot._series_dict short-circuited count==0 to {} before _series_to_metric_dict could keep the all-null early_stopping_percentiles map. Now delegates unconditionally, and the regression test pins Report.from_snapshot itself rather than the helper (the previous test's false-confidence trap).
• medium / error-handling: ES is now best-effort on the finalize path — _es_estimates catches, logs, and omits the map instead of letting an exception escape publish_final after the finalized guard is set (which would cost the entire final report).
• medium / documentation: the _betacf iteration-cap comment cited a fabricated 3e-8 accuracy figure; replaced with the correct rationale (safety comes from the odds ≈ 0.01 decision boundary — midpoint accuracy in the cap regime is explicitly not claimed, and is now pinned by a test).
• low / parity: find_min_passing's boundary is now strict (odds < 1 − c), byte-matching loadgen/early_stopping.cc:70.
• low: _discard_count docstring off-by-one wording; script gap lines gated on is not None (a legitimate 0.0 empirical was silently dropped) with zero-division guarded separately; EarlyStoppingResult converted to a frozen slots dataclass like its sibling spec.
• testing: _betai both branches pinned against closed forms (I_x(a,1) = x^a, I_x(1,b) = 1−(1−x)^b), the iteration-cap regime pinned sane, and the t=1 no-discard floor boundary pinned (estimate == max, discarded == 0).

2. 9c88b33 — one percentile convention: grid (0–100) at every surface

The codebase mixed 99.9 (grid, np.percentile, summary keys) and 0.999 (the whole ES surface), stitched together by two different rounding hacks (round(p/100, 6) and round(p*100, 4)) and a key-reconstruction helper. Now:

• EarlyStoppingSpec.percentiles, es_percentile_estimate(percentile=…), EarlyStoppingResult.percentile, ES_MIN_PERCENTILE (= 50.0), es_targets_from_grid values, and the script's --percentiles all speak 0–100.
• es_targets_from_grid does no conversion and no rounding at all — keys are str() of the original grid entries, values the entries themselves.
• grid_percentile_key is deleted; both rounding constants are gone.
• The LoadGen-parity kernel (find_min_passing/_odds/_discard_count) keeps LoadGen's own fraction domain; the single conversion lives inside es_percentile_estimate, unrounded — the same /100 that np.percentile applies internally, so empirical-vs-grid parity is exact by construction.
• Estimates are bit-identical before/after (interface-only change). This also closes the round-3 question about the 6-vs-4 rounding inconsistency — both roundings no longer exist.

Gate: 1340 unit tests, 13/13 pre-commit hooks, LoadGen reference anchors (459/44/661) unchanged.

🤖 Generated with Claude Code

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[github-actions]

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

[nvzhihanj]

Review round (Codex + Claude, both on eb7fc7b)

Codex (codex review --base origin/main): no findings — it ran the ES unit suites and probed the float/rounding behavior of the convention switch directly.

Claude reported 3 findings; each was verified against HEAD before fixing, all addressed in 50e02bf:

1. medium / api-contract — silent fraction-style footgun. After the convention switch, a pre-refactor(metrics): unify the percentile convention; address #417 round-3 review #423 invocation like --percentiles "0.99" is valid input meaning the sub-1% percentile p0.99 — it would produce a plausible-looking sufficient=true block instead of failing. The script now hard-errors on values < 1 with a did-you-mean message (0.99 → did you mean 99.0?), and es_percentile_estimate's docstring calls out the hazard. Sub-1% targets have no use here (ES is a tail certification; the product path filters at ES_MIN_PERCENTILE = 50), so rejecting is strictly safer than guessing. Test added: fraction-style input raises SystemExit.
2. low / correctness — CLI fallback targets didn't dedupe on value. --percentiles "99,99.0" produced two identical computations under two keys ("99", "99.0"). Fallback keys are now canonical str(float(...)), deduped by parsed value, descending to match the grid order. Test added. (The product path is unaffected — grid entries are already canonical.)
3. low / documentation — stale comment. A snapshot.py comment still referenced grid_percentile_key, deleted by this PR. Reworded.

Claude also verified the core claims of the refactor negatively: the strict-vs-lenient boundary behavior of find_min_passing is identical across the domain change, and the single unrounded /100 matches np.percentile's internal division on all grid values tested up to 1e6.

Gate on 50e02bf: 1341 unit tests pass, pre-commit green.

🤖 Generated with Claude Code