Skip to content

fix: WeakspotsDiagnosis crash on custom metrics subset#541

Open
hunner wants to merge 1 commit into
mainfrom
agents/sc-17267-weakspots-threshold-subset
Open

fix: WeakspotsDiagnosis crash on custom metrics subset#541
hunner wants to merge 1 commit into
mainfrom
agents/sc-17267-weakspots-threshold-subset

Conversation

@hunner

@hunner hunner commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Pull Request Description

What and why?

WeakspotsDiagnosis raises ValueError: Unable to coerce to Series, length must be 1: given 4 when called with a custom metrics= dict that's a strict subset of the default metric names (accuracy/precision/recall/f1) and no explicit thresholds=.

The pass/fail check narrows pass_columns to the metrics actually requested, but compares against pass_thresholds, which still carries all four default keys:

pass_columns = [c for c in pass_thresholds if c in metrics]   # e.g. ["F1"]
df[pass_columns].lt(pass_thresholds)                          # pass_thresholds has 4 keys

DataFrame.lt(dict) requires the dict to match the DataFrame's column count in this usage, so a 1-column frame against a 4-key dict raises. Minimal standalone repro:

import pandas as pd
df = pd.DataFrame({"F1": [0.9, 0.2]})
df[["F1"]].lt({"Accuracy": 0.75, "Precision": 0.5, "Recall": 0.5, "F1": 0.7})
# ValueError: Unable to coerce to Series, length must be 1: given 4

This is a plausible, unremarkable usage pattern (a caller who only cares about F1) rather than an edge case anyone would think to avoid — passing a matching thresholds= sidesteps it, but nothing signals that requirement.

Found as a side-effect of implementing sc-17261 (the ZD-704/#534 follow-up); tracked separately as sc-17267 since it's an independent bug, not tied to that customer ticket.

What changed

Narrows pass_thresholds to pass_columns right before the comparison. plot_thresholds (used for chart reference lines) is untouched, so charts still show reference lines for every metric — only the pass/fail check is scoped to what was actually requested.

How to test

from validmind.tests.model_validation.sklearn.WeakspotsDiagnosis import WeakspotsDiagnosis
from sklearn.metrics import f1_score
# ... datasets/model set up ...
WeakspotsDiagnosis(datasets=[train_ds, test_ds], model=vm_model,
                   metrics={"f1": f1_score})   # no thresholds= — used to crash

The added test builds a case where the default (all-four-metric) run fails on Accuracy while an F1-only run passes on the same data — proving the fix actually scopes pass/fail correctly rather than just suppressing the crash. pytest tests/unit_tests/model_validation/sklearn/test_WeakspotsDiagnosis.py -q → 15 passed.

What needs special review?

The distinguishing test relies on binary data where F1 ≥ 0.7 mathematically forces precision/recall above their own 0.5 thresholds, so Accuracy is the only metric that can independently fail — worth a glance to confirm that reasoning holds.

Dependencies, breaking changes, and deployment notes

None. Default (all-metrics) and full-thresholds-supplied paths are unaffected — this only changes behavior for the previously-crashing subset-without-matching-thresholds case.

Release notes

Fixed WeakspotsDiagnosis raising ValueError: Unable to coerce to Series, length must be 1: given 4 when called with a custom metrics= subset and no matching thresholds=.

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-17267

🤖 Generated with Claude Code

When a custom metrics= dict is a strict subset of the default metric
names and no thresholds= is supplied, pass_thresholds still carried all
four default keys while pass_columns narrowed to the subset, so the
df[pass_columns].lt(pass_thresholds) comparison raised
"Unable to coerce to Series, length must be 1: given 4". Narrow the
thresholds to pass_columns before the comparison; plot_thresholds is
left untouched so charts keep reference lines for all metrics.

Co-Authored-By: Claude Sonnet 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 introduces several enhancements to the WeakspotsDiagnosis component by refining its handling of custom metrics and threshold comparisons. The changes are twofold:

  1. In the unit tests (in tests/unit_tests/model_validation/sklearn/test_WeakspotsDiagnosis.py), three new test cases are added along with a helper function (_all_positive_pair) to generate deterministic datasets. These tests verify:

    • That providing a custom metrics dictionary (a subset of the default metrics) works as expected without triggering errors due to mismatched threshold entries.
    • That the pass/fail outcome is based solely on the custom metric (F1 score) when specified, showing a case where the default composite fails while the F1-only evaluation passes.
    • That thresholds provided for metrics not being computed are safely ignored, ensuring that the evaluation outcome reflects only the metrics in scope.
  2. In the main logic (in validmind/tests/model_validation/sklearn/WeakspotsDiagnosis.py), the threshold check has been updated. Instead of comparing all default metrics with their thresholds, the code now filters the thresholds to only include those metrics that are actually being evaluated (pass_columns). This improves the robustness and correctness of the pass/fail determination logic.

Overall, these changes increase the flexibility in defining and evaluating metric thresholds, making the component more robust when using customized scoring logic.

Test Suggestions

  • Add tests to verify behavior when multiple feature columns are provided along with their custom metrics and thresholds.
  • Include tests where the dataset values are exactly at threshold boundaries to ensure that comparisons behave as expected.
  • Test behavior with an empty or missing custom metrics dictionary to confirm fallback to default behavior.
  • Check that thresholds for non-requested metrics are indeed ignored and do not affect the pass/fail outcome.

@juanmleng

Copy link
Copy Markdown
Contributor

Nice fix — I traced the pass/fail path end to end and it holds up. Narrowing the threshold dict to pass_columns right before the comparison means the dict and the selected columns are always built from the same keys, so the length mismatch that raised Unable to coerce to Series, length must be 1: given 4 can't come back. The default all-metrics path and the explicit-thresholds path both stay exactly as they were, and leaving plot_thresholds alone keeps reference lines on the charts for every metric.

The tests are the part I liked most — they don't just prove the crash is gone, they engineer the data so the default run fails on Accuracy while the F1-only run passes, which actually proves the pass/fail is scoped to the requested metric and not just silently suppressed. I re-checked that against the default thresholds and it lines up.

Two small, non-blocking observations, both pre-existing rather than anything this PR introduced:

  • If someone passes a fully custom metric name that isn't one of the four defaults and doesn't supply a threshold for it, the pass/fail check ends up empty and the result stays "passed" by default. That's arguably the right behavior (no threshold means nothing to fail against), but it's silent — might be worth a docstring note down the line.
  • Your "what needs special review" call-out checks out: on that binary data an F1 ≥ 0.7 does force precision/recall above their 0.5 thresholds, so Accuracy is the only metric that can fail independently. No change needed.

Approving — thanks for the thorough test.

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.

2 participants