Fix per-key validation metrics judging every key by the whole target dict - #9
Closed
arthi-arumugam-git wants to merge 1 commit into
Closed
Conversation
…dict compute_validation_metrics computes target_positive once from the entire target with is_positive_value(entry.target), then reuses it for every key of a dict-valued validation. Dict- and label-based validation store the target as a dict keyed like valid (see _validate_dict and the labels path in _scan), and is_positive_value of any non-empty dict is always True. So every key is judged against a positive expectation: tn and fp can never be recorded, per-key precision is pinned at 1.0, and specificity and balanced accuracy are always None, regardless of how many false positives occur. Judge each key against its own target, is_positive_value(entry.target[key]) when target is a dict, falling back to the scalar target for the legacy scalar-target-with-dict-valid shape. The existing dict tests all use a scalar target, so they never exercised the production shape; add tests with dict and label targets carrying a negative per-key expectation.
Author
|
Closing this myself, since asking you to review a patch that upstream already shipped is not a good use of your time. For the record, in case it is useful to whoever maintains this fork:
So the fix worth making is a sync, or a cherry-pick of |
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.
Read this first: upstream already fixed this
This repository is a fork of
meridianlabs-ai/inspect_scout, and upstream fixed this exact bug on 2026-06-13 inaff8db4d("Bugfix: per-key validation metrics use per-key target positivity for dict targets"), with a follow-up type fix in551ec394. I did not know that when I wrote the patch below, and I found it while checking whether the fix had already landed somewhere. The change here is functionally the same as upstream's.This fork's
mainis at a 2026-03-25 snapshot and is roughly 350 commits behind upstream, which is why the bug is still present here.Given that, you have three reasonable options:
aff8db4ddirectly, which matches the existingCherry-pick: ...convention already used on this branch (seec724b927).I would pick 1 or 2 over 3. I am leaving this open rather than deleting it because the bug report and the reproduction are still accurate for this fork's current
main, and because option 1 is a decision only you can make. Close it without ceremony if you would rather sync.The rest of this description is the original analysis, which still describes the state of
mainhere.What
In
compute_validation_metrics(src/inspect_scout/_recorder/validation.py), the per-key branch computestarget_positiveonce from the whole target and reuses it for every key:Dict- and label-based validation store
targetas a dict keyed likevalid(_validate_dict, and the labels path storestarget=v_case.labelsin_scan).is_positive_valueof any non-empty dict is alwaysTrue(only{}is falsy). So on the dict path every key is judged against a positive expectation:_update_metricscan only reach thetarget_positive=Truebranches, sotnandfpare never recorded;precision = tp/(tp+fp)is pinned at 1.0 whenevertp > 0, and isNoneotherwise;specificity = tn/(tn+fp)and balancedaccuracyare always None.This holds no matter how many false positives a scanner produces. Concretely, for a key whose target is
False:Note the direction: because
validmeans "validation passed" rather than "the scanner fired", the wrongly-flagged case lands infn, nottp. Either way the negative half of the confusion matrix is unreachable, which is what pins precision and erases specificity.Reproduction
Production shape (dict target keyed like
valid), where keybis expected absent:bWhy it slipped through
The existing dict tests (
tests/recorder/test_summary.py) pass a scalartargetwith a dictvalid, e.g.ValidationEntry(target=True, valid={"a": True, "b": False}). That combination never occurs in production (a dictvalidalways comes with a dicttarget), and on a scalar targetis_positive_valuebehaves correctly, so the tests stayed green.Fix
Judge each key against its own target:
is_positive_value(entry.target[key])when the target is a dict, falling back to the scalar target for the legacy scalar-target shape (keeping the existing tests valid). Added tests covering dict targets and label targets with a negative per-key expectation.This is the same approach upstream took in
aff8db4d. Upstream also added aCHANGELOG.mdentry, which this PR does not, since this fork's changelog has diverged.ruff check,ruff format --checkclean.