Skip to content

fix: CalibrationCurve non-{0,1} binary crash and Weakspots custom-metric override#542

Open
hunner wants to merge 2 commits into
mainfrom
agents/sc-17261-calibration-custom-metrics
Open

fix: CalibrationCurve non-{0,1} binary crash and Weakspots custom-metric override#542
hunner wants to merge 2 commits into
mainfrom
agents/sc-17261-calibration-custom-metrics

Conversation

@hunner

@hunner hunner commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Pull Request Description

What and why?

Two latent bugs found while empirically reviewing PR #534 (the ZD-704 multiclass fix, shipped in v2.13.6/v2.13.7), reported in that PR's approving review but merged unaddressed. Neither affects the ZD-704 customer (multiclass model, default metrics) — both are follow-on issues of the same family. Tracked as sc-17261.

1. CalibrationCurve crashes on binary targets encoded outside {0, 1}

#534 added a multiclass-only skip (len(np.unique(dataset.y)) > 2SkipTestError), but a two-class target whose labels aren't {0, 1} (e.g. {0, 4}, string labels) sails past that guard into sklearn's calibration_curve with no pos_label, hitting the exact cryptic error the guard was meant to eliminate:

ValueError: y_true takes value in {0, 4} and pos_label is not specified: either make y_true take value in {0, 1} or {-1, 1} or pass pos_label explicitly.

2. WeakspotsDiagnosis silently overrides user-supplied custom metrics

apply_averaging (from #534's shared _diagnosis_metrics.py) rebinds every callable in the metric registry that exposes an average parameter — including user-supplied ones. functools.partial(f1_score, average="weighted") still exposes average via inspect.signature, so it gets silently rebound to the resolved averaging (e.g. macro), changing the reported score without any error:

p = functools.partial(f1_score, average="weighted")
bind_averaging(p, "macro", None)(y_true, y_pred)  # returns macro, not the user's weighted score

What changed

  • CalibrationCurve.py — passes pos_label=np.unique(dataset.y).max() to calibration_curve, but only when the installed sklearn's signature exposes pos_label (added in scikit-learn 1.1; the library's pyproject.toml has no lower bound, so an unconditional pass would break {0,1} targets on older installs — a regression). The guard mirrors the existing signature-inspection convention in _diagnosis_metrics.bind_averaging. {0,1} behavior is byte-identical (verified against raw calibration_curve output).
  • WeakspotsDiagnosis.py — averaging rebinding now only applies when the caller didn't pass custom metrics (i.e. only the defaults get rebound); custom callables keep their own kwargs.

How to test

PYTHONPATH=<worktree> pytest tests/unit_tests/model_validation/sklearn/test_CalibrationCurve.py tests/unit_tests/model_validation/sklearn/test_WeakspotsDiagnosis.py tests/unit_tests/model_validation/sklearn/test_OverfitDiagnosis.py tests/unit_tests/model_validation/sklearn/test_RobustnessDiagnosis.py -q

New coverage: CalibrationCurve on {0,4} and string-label binary (no raise) plus {0,1} unchanged (compared against raw calibration_curve); WeakspotsDiagnosis with a custom partial(f1_score, average="weighted") metric asserting the weighted value lands in the results (with a check that weighted ≠ macro on the test data, so the assertion is meaningful), plus confirmation that default runs still get macro/pos_label rebinding for multiclass and non-{0,1} binary. 41 tests passed across the touched + adjacent (Overfit/Robustness) modules; the three new tests were confirmed to fail against unfixed source with the exact reported errors before the fix, then pass after.

What needs special review?

  • The pos_label=np.unique(dataset.y).max() convention follows resolve_averaging's existing "largest label is positive" rule from Fix multiclass / non-{0,1}-label crashes across the sklearn classification tests (ZD-704) #534 — for string labels this is lexicographic max ("good" > "bad"), worth a sanity check against expectations.
  • The sklearn-version guard (inspect.signature check for pos_label) rather than an unconditional pass — deliberate, to avoid breaking {0,1} on pre-1.1 sklearn given the unbounded pyproject.toml constraint; CI's actual resolved floor (scikit-learn 1.6.1 on Python 3.9 per uv.lock) means this costs nothing there but protects the documented floor.
  • OverfitDiagnosis/RobustnessDiagnosis take a metric name string rather than callables, so they're unaffected by the custom-metrics-override issue — confirmed while implementing, not otherwise touched.

Dependencies, breaking changes, and deployment notes

None. {0,1} CalibrationCurve and default-metric WeakspotsDiagnosis behavior are unchanged; only the two previously-broken/incorrect paths change.

Separately, implementing this surfaced an unrelated pre-existing bug in WeakspotsDiagnosis's pass/fail check (crashes when a custom metrics= subset is passed without matching thresholds=) — filed and fixed independently as sc-17267 / PR #541, not part of this PR.

Release notes

Fixed CalibrationCurve raising a sklearn pos_label error on binary targets not encoded as {0, 1}, and fixed WeakspotsDiagnosis silently overriding user-supplied custom metric averaging (e.g. a partial(f1_score, average="weighted") was previously rebound to macro without warning).

Checklist

  • What and why
  • How to test
  • What needs special review
  • Dependencies, breaking changes, and deployment notes
  • Labels applied
  • PR linked to Shortcut
  • Unit tests added
  • Tested locally
  • Documentation updated (if required)
  • Environment variable additions/changes documented (if required)

Closes sc-17261

🤖 Generated with Claude Code

ValidMind Support Agent and others added 2 commits July 20, 2026 11:22
A two-class target encoded outside {0, 1} (e.g. {0, 4} or string labels)
passed the multiclass guard but hit sklearn's calibration_curve without a
pos_label, raising "y_true takes value in {0, 4} and pos_label is not
specified". Resolve the positive class to the largest label, matching the
convention in _diagnosis_metrics.resolve_averaging. pos_label landed in
scikit-learn 1.1, so it is bound only when calibration_curve's signature
exposes it; for {0, 1} the curve is byte-identical to the previous default.

Found during the empirical review of PR #534 (ZD-704, shipped in v2.13.7).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
apply_averaging rebinds any callable whose signature exposes `average`,
including user metrics like functools.partial(f1_score, average="weighted")
(a partial still exposes the parameter), silently replacing the caller's
averaging with the resolved default. Only rebind when the caller did not
supply custom metrics; custom callables own their kwargs. The defaults path
is unchanged, so multiclass/{0,4} targets still get macro/binary rebinding.

OverfitDiagnosis/RobustnessDiagnosis take a metric name string, not
callables, so they are unaffected.

Found during the empirical review of PR #534 (ZD-704, shipped in v2.13.7).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@hunner hunner added the bug Something isn't working label Jul 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Summary

This PR enhances both the CalibrationCurve and WeakspotsDiagnosis functionality to better handle non-standard binary encodings and custom metrics. Key changes include:

  • Adding new unit tests in CalibrationCurve to verify that binary models with non-standard target encodings (e.g., {0, 4} or string labels) no longer trigger errors from scikit-learn due to missing pos_label specification. The tests confirm that backward compatibility is maintained when labels are {0, 1} by ensuring byte-identical results with sklearn's default calibration curve behavior.

  • Enhancing the CalibrationCurve function to inspect the signature of sklearn's calibration_curve function and conditionally pass the pos_label parameter. This is to avoid errors on scikit-learn versions that support the parameter while maintaining functionality on older versions.

  • Updating WeakspotsDiagnosis to preserve the averaging options for custom user-supplied metrics. The changes ensure that default metrics are rebound (to resolve issues with multiclass/binary handling) but that custom metrics (e.g., using functools.partial with a fixed averaging) retain their original configuration. New tests verify that the custom metric calculations remain true to the user's intent, while default metrics still follow the rebinding logic.

Test Suggestions

  • Test CalibrationCurve with additional non-standard binary labels, including negative integers and mixed types.
  • Simulate an environment with an older scikit-learn version (pre-1.1) to ensure that the calibration_curve works correctly without pos_label.
  • Add tests to ensure that custom metrics using functools.partial retain their user-defined averaging and are not rebinding unexpectedly.
  • Validate that the fallback for default metrics still provides the expected macro-averaged values for multiclass scenarios.

@hunner
hunner requested a review from juanmleng July 20, 2026 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant