fix(metrics): share one guarded scoring loop between calculate_metrics and compute_metrics - #874
Open
Maarmapa wants to merge 1 commit into
Open
Conversation
… metric
`calculate_metrics` and `compute_metrics` carried a copy each of the same
scoring loop, and the copies drifted: only the first dropped non-finite
scores. The second is the one the CV evaluation loop calls to build the
objective of an HPO trial, so a metric too suspect to write to the database
was still trusted to decide which hyperparameters won -- and every comparison
against a NaN is False in both directions, so that trial could neither be
beaten nor win.
Both now delegate to `BaseModel._score_split`, which owns the data loading,
the prediction, the scoring and the `math.isfinite` guard. It returns None
when there was nothing to score and a dict otherwise, which lets the two
callers keep their different contracts: `calculate_metrics` persists an
empty result (every metric was asked and none was usable) but skips when
there was nothing to ask, while `compute_metrics` collapses both to `{}`.
Second half: `cv.py` indexed the fold's validation scores with the goal
metric's name and trusted it to be there. It is not always -- the metric may
not be among the run's validation metrics, and now that non-finite scores are
dropped it can also be missing because it was undefined on that fold. Both
are configuration errors, and both surfaced as a bare KeyError several frames
below where the choice was made. It now raises with the metric asked for and
the metrics actually scored.
RuntimeError and not ValueError on purpose: `study.optimize` runs with
`catch=UNFITTABLE_TRIAL_ERRORS`, which includes ValueError, so a ValueError
here would be swallowed into "all N trials failed, narrow the ranges and try
again". The same reasoning is already written into optuna_optimizer.py, where
an unsupported parameter type raises TypeError for this exact reason. A test
asserts the exception is not one the optimizer catches.
Closes DashAISoftware#843
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BzZ6td4VJdRDe76UndWZY3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… metric
calculate_metricsandcompute_metricscarried a copy each of the same scoring loop, and the copies drifted: only the first dropped non-finite scores. The second is the one the CV evaluation loop calls to build the objective of an HPO trial, so a metric too suspect to write to the database was still trusted to decide which hyperparameters won -- and every comparison against a NaN is False in both directions, so that trial could neither be beaten nor win.Both now delegate to
BaseModel._score_split, which owns the data loading, the prediction, the scoring and themath.isfiniteguard. It returns None when there was nothing to score and a dict otherwise, which lets the two callers keep their different contracts:calculate_metricspersists an empty result (every metric was asked and none was usable) but skips when there was nothing to ask, whilecompute_metricscollapses both to{}.Second half:
cv.pyindexed the fold's validation scores with the goal metric's name and trusted it to be there. It is not always -- the metric may not be among the run's validation metrics, and now that non-finite scores are dropped it can also be missing because it was undefined on that fold. Both are configuration errors, and both surfaced as a bare KeyError several frames below where the choice was made. It now raises with the metric asked for and the metrics actually scored.RuntimeError and not ValueError on purpose:
study.optimizeruns withcatch=UNFITTABLE_TRIAL_ERRORS, which includes ValueError, so a ValueError here would be swallowed into "all N trials failed, narrow the ranges and try again". The same reasoning is already written into optuna_optimizer.py, where an unsupported parameter type raises TypeError for this exact reason. A test asserts the exception is not one the optimizer catches.Closes #843
Claude-Session: https://claude.ai/code/session_01BzZ6td4VJdRDe76UndWZY3
Summary
Short explanation of what changed and why.
Type of Change
Check all that apply like this [x]:
Changes (by file)
Briefly list the important modified files and what was done.
Example:
path/to/file.py: descriptionanother/file.tsx: descriptionTesting (optional)
Only add if there's something reviewers should verify.
Notes (optional)
Additional context or considerations.