Skip to content

Fix per-key validation metrics judging every key by the whole target dict - #9

Closed
arthi-arumugam-git wants to merge 1 commit into
METR:mainfrom
arthi-arumugam-git:fix-per-key-validation-target
Closed

Fix per-key validation metrics judging every key by the whole target dict#9
arthi-arumugam-git wants to merge 1 commit into
METR:mainfrom
arthi-arumugam-git:fix-per-key-validation-target

Conversation

@arthi-arumugam-git

@arthi-arumugam-git arthi-arumugam-git commented Aug 11, 2026

Copy link
Copy Markdown

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 in aff8db4d ("Bugfix: per-key validation metrics use per-key target positivity for dict targets"), with a follow-up type fix in 551ec394. 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 main is 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:

  1. Sync the fork with upstream. That fixes this and 350 commits of other things, and makes this PR unnecessary.
  2. Cherry-pick aff8db4d directly, which matches the existing Cherry-pick: ... convention already used on this branch (see c724b927).
  3. Merge this PR, which is the same fix with somewhat broader tests.

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 main here.


What

In compute_validation_metrics (src/inspect_scout/_recorder/validation.py), the per-key branch computes target_positive once from the whole target and reuses it for every key:

for entry in with_targets:
    target_positive = is_positive_value(entry.target)   # whole dict
    if isinstance(entry.valid, dict):
        for key, valid in entry.valid.items():
            _update_metrics(per_key[key], target_positive, valid)

Dict- and label-based validation store target as a dict keyed like valid (_validate_dict, and the labels path stores target=v_case.labels in _scan). is_positive_value of any non-empty dict is always True (only {} is falsy). So on the dict path every key is judged against a positive expectation:

  • _update_metrics can only reach the target_positive=True branches, so tn and fp are never recorded;
  • per-key precision = tp/(tp+fp) is pinned at 1.0 whenever tp > 0, and is None otherwise;
  • specificity = tn/(tn+fp) and balanced accuracy are always None.

This holds no matter how many false positives a scanner produces. Concretely, for a key whose target is False:

  • the scanner correctly leaving it unflagged (validation passed) is booked tp when it should be tn;
  • the scanner wrongly flagging it (validation failed) is booked fn when it should be fp.

Note the direction: because valid means "validation passed" rather than "the scanner fired", the wrongly-flagged case lands in fn, not tp. 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 key b is expected absent:

key b tp fp tn fn precision specificity
before 1 0 0 1 1.0 None
correct 0 1 1 0 0.0 0.5

Why it slipped through

The existing dict tests (tests/recorder/test_summary.py) pass a scalar target with a dict valid, e.g. ValidationEntry(target=True, valid={"a": True, "b": False}). That combination never occurs in production (a dict valid always comes with a dict target), and on a scalar target is_positive_value behaves 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 a CHANGELOG.md entry, which this PR does not, since this fork's changelog has diverged.

ruff check, ruff format --check clean.

…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.
@arthi-arumugam-git

Copy link
Copy Markdown
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:

  • The bug is real and is still present on this fork's main. Per-key validation metrics judge every key against the whole target dict, which is always truthy, so tn and fp are never recorded, per-key precision is pinned at 1.0, and specificity and balanced accuracy are always None.
  • Upstream fixed it on 2026-06-13 in aff8db4d, with a follow-up type fix in 551ec394.
  • This fork's main is at a 2026-03-25 snapshot, roughly 350 commits behind meridianlabs-ai/inspect_scout, which is why it is still here.

So the fix worth making is a sync, or a cherry-pick of aff8db4d in the style of c724b927, rather than anything in this branch. Happy to open that cherry-pick as a one-line PR if you would find it useful, otherwise there is nothing to action here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant