Fix zero-valued LER handling in validation metrics#84
Conversation
Preserve valid zero-valued logical error rates when selecting metric aliases and averaging X/Z results. Add regression coverage for both-basis, mixed, and top-level zero metrics. Signed-off-by: huaweil <huaweil@nvidia.com>
When LER extraction fails (or PREDECODER_LER_FINAL_ONLY=1 skips it), early stopping falls back to validation loss, but checkpoint metadata still recorded using_ler=True. On resume, that flag decides whether the saved best_vloss is comparable to the current metric, so a loss value could be restored and compared against LER values (different scales). - Save side: record using_ler only when LER actually produced the saved metric (mirrors the current_metric selection). - Resume side: expect a loss-based best when PREDECODER_LER_FINAL_ONLY=1, matching what the per-epoch loop will actually compare against. Complements the zero-LER extraction fix: this was the fourth symptom listed in the PR description (checkpoint metadata still recorded using_ler=True). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ivanbasov
left a comment
There was a problem hiding this comment.
Reviewed — the fix is correct and well-scoped.
What I checked:
- The
_first_present_metrichelper and the rewritten X/Z basis selection correctly preserve0.0while keeping the original key-priority order and the both-bases averaging semantics. The top-level key scan is also strictly better than before: a key explicitly set toNoneno longer shadows a later valid alias. - All 13 tests in
test_metrics_extras.pypass locally, plus the checkpoint-resume and training test suites. - Swept the rest of the codebase for the same falsy-zero pattern (
or-chains and truthiness checks on LER values):_extract_reduction_factor, the multi-pair aggregation and allvalidation_lerconsumers intrain.py, the CI log parsers, andinference.pyare all clean (is not Nonethroughout). The> 0guards inthreshold_plot.pyare intentional (log-scale plotting).
One symptom from the description was left unfixed, so I pushed f0ea8b6 to this branch rather than requesting changes: checkpoint metadata recorded using_ler=True even on epochs where LER extraction failed and early stopping fell back to validation loss. Since resume uses that flag to decide whether the saved best_vloss is comparable, a loss value could later be restored and compared against LER values. The commit records the metric that actually produced best_vloss, and makes the resume-side check aware of PREDECODER_LER_FINAL_ONLY=1 (where the per-epoch metric is always loss) so resume in that mode keeps restoring correctly.
|
/ok to test f0ea8b6 |
|
Thanks a lot for this contribution, @huaweil-nv! Great catch on the falsy-zero |
ivanbasov
left a comment
There was a problem hiding this comment.
LGTM. The zero-LER extraction fix is correct and complete, the codebase sweep found no other instances of the falsy-zero pattern, and the checkpoint-metadata follow-up (f0ea8b6) closes the last symptom from the description. All affected test suites pass locally.
Summary
Preserve valid logical error rate values of
0.0during validation metric extraction.Previously, LER aliases were selected using boolean
orchains. Because0.0is falsy in Python, a perfect LER could be skipped and ultimately treated as missing.Impact
When both X and Z validation LERs were
0.0:using_ler=True.This could prevent a zero-error model from being selected using the configured LER metric.
Validation
Verified through the documented
local_run.shtraining path on an NVIDIA A100:Use a fresh experiment name to avoid loading artifacts from an earlier run:
Before
After