fix(utils): mean_of reports nan rather than 0.0 when nothing was averaged - #2230
MattFisher merged 4 commits into
Conversation
…aged mean_of had two zero returns. The leading `if not scores: return 0.0` is unreachable from the framework, which does not invoke a metric when every sample is unscored. The one at the end is reachable in an otherwise healthy run: with on_missing="skip", a sample missing the key never increments count, so if no sample carries it the metric reports 0.0 with nothing to say it measured nothing. 0.0 is a real value on every scale these metrics use, so the caller cannot tell an all-miss run from an unmeasured one. Reported as nan instead, matching agentharm's avg_score_non_refusals after UKGovernmentBEIS#2174. The unreachable guard is removed rather than changed, since the loop already handles an empty list: count stays 0 and falls to the same return, so direct callers (unit tests, one metric delegating to another) get nan too. Two existing assistant_bench tests asserted the old 0.0 on an empty list and now assert nan.
This comment has been minimized.
This comment has been minimized.
MetricProtocol returns Value, the union of every shape a metric can report, which mypy will not accept as an argument to math.isnan. Adds the same narrowing helper tests/agentharm/test_metric.py already uses. Also files the changelog fragment under Other. Shared Utilities is not one of the three categories declared in pyproject, so the entry would not have been collected into the release notes.
This comment has been minimized.
This comment has been minimized.
MattFisher
left a comment
There was a problem hiding this comment.
Approving, with the version-bump question you raised answered below. This fix is close to a worked example of the convention it aligns with: the on_missing="skip" all-miss case is the live, internally-emptied, fallen-into shape (nothing pinned either 0.0); removing the dead guard so the fall-through returns nan for direct callers matches the true-empty-value rule for a quotient-shaped metric; and the legitimate zeros stay pinned. Red-green shown, the two deliberately-flipped legacy assertions are flagged rather than silently rewritten, and 347 tests across the affected suites pass.
Version bumps — please add them for the five skip-site evals (assistant_bench, browse_comp, cyberseceval_4, mind2web, tac), one-line README entries each. The repo precedent (settled on #2188 and applied through the cluster) is bump where reported numbers can move, and the all-miss case moves them — the same shape #2174 and #2179 bump for. The other ~36 call sites need nothing: their behaviour changes only on the framework-unreachable empty path. One sequencing note: cyberseceval_4's eval.yaml will collide with #2179 — whichever lands second rebumps, same as the agentharm pair.
Upstream alignment, verified — this fix converges with where #2122 is headed. inspect_ai.scorer.aggregate() (the planned mean_of replacement) already returns NaN for all-skipped, empty-list, and all-NaN-leaf inputs — probed empirically on the current pin, and deliberate per its docstring ("matches the Score.unscored() / NaN sentinel used elsewhere in the framework"). Until this PR, upstream's claim that "a mean_of → aggregate swap preserves behaviour" was subtly false for the empty case; after it, the #2122 migration becomes genuinely behaviour-preserving.
One nuance to hand to #2122 rather than this PR: upstream's docstring warns that on_missing="skip" makes stderr/std depend on the missing-key rate (two runs with identical variance report different stderrs if miss rates differ), recommending "zero" for a constant denominator. None of the seven skip sites has considered that question — worth a per-site decision at migration time.
Posted by Claude Code on Matt's behalf.
MattFisher
left a comment
There was a problem hiding this comment.
Sorry that should have been requested changes
|
Could you also raise an issue for this?
|
The nan change reaches assistant_bench, browse_comp, cyberseceval_4, mind2web and tac, whose metrics pass on_missing="skip" to mean_of. Per the precedent settled on UKGovernmentBEIS#2188, bump N where reported numbers can move, and the all-miss case moves these. assistant_bench 4-B -> 5-B browse_comp 2-B -> 3-B cyberseceval_4 4-B -> 5-B mind2web 2-A -> 3-A tac 6-C -> 7-C Each gets a one-line README changelog entry. The scriv fragment now names the five under Existing Evals, alongside the shared-utility entry that stays under Other. The remaining call sites are untouched: they use on_missing="error" or "zero", so their behaviour changes only on the empty-scores path the framework does not invoke. cyberseceval_4's eval.yaml will collide with UKGovernmentBEIS#2179. Whichever lands second rebumps.
This comment has been minimized.
This comment has been minimized.
The TAC entry carried its house-style comparability line, which claims results are not comparable to 6-C and require a re-run. That is not true. tac_scorer sets welfare and completed on every one of its five return branches, so no sample can be missing the key and the all-miss path cannot arise. Re-running a 52-sample agentic eval for that is a real cost for no change. The same holds for assistant_bench, browse_comp and mind2web: every return branch sets the key. Each entry now says so. cyberseceval_4 is the one where it is genuinely reachable, because bleu_score is only recorded when a reference can be built, and its entry already says so. The bumps stay as requested. They declare the shared-helper change rather than describing a number that moved.
Claude Code ReviewClaude Code ReviewSummaryFollow-up review. One commit since my last review: I re-verified the reachability claims the new README text now asserts, because the branch depends on them being true: Also re-checked the mechanical constraints: all five Issues FoundRelease-notes fragment no longer matches the README entries it summarises
The release notes are the artefact most users see, and four of the five lines currently invite a reader to wonder whether their numbers moved. Suggest a short clause on each of the four (e.g. "the eval's scorer sets the key on every path, so no reported number changes; the bump declares the shared-helper change"), or one shared sentence covering the four. Location: For the maintainer
Previously raised, still no response and no code change
Reviewer Feedback Status
Notes
This is an automatic review performed by Claude Code. Any issues raised here should be fixed or justified, but a human review is still required in order for the PR to be merged. Maintainers: comment |
|
Thanks, and no worries about the approve then request-changes, I knew what you meant. Bumps are pushed in
I've ticked the two checklist boxes and removed the note asking the question, since you've answered it. The scriv fragment now names the five under On Issue raised: #2231. One thing I should flag before you read it, because it's about the examples in your comment. I checked all three rather than just Mind2Web, and two of them aren't quite what the comment says:
And one more, which might change how you feel about the five bumps. I went looking for how an all-miss actually arises, and I could only find a path in That turned up something I got wrong on the first push. I'd copied TAC's house-style line saying results are not comparable to I've kept all five bumps as you asked, and I'd rather over-declare than under-declare. But four of the five are declarations of the shared-helper change rather than descriptions of a number that moved, so if you'd rather trim the list to just Your point about Tests: |
Two things from the latest review.
Correctness: the four field patterns replaced the literal ': ' with ':\s*\**\s*',
and \s matches \n. A label emitted with no value on its own line therefore
skipped the blank line and bound to the *next* field, so
extracted_final_answer:
reasoning: the years do not match
set Score.answer to 'reasoning: the years do not match'. On main the ': '
requirement simply did not match and answer fell back to 'None'. That is the
same failure this PR exists to fix - a parser returning a confident wrong value
instead of not matching - so it should not have been introduced here.
All four now use horizontal-only whitespace ([^\S\n]), which keeps the
capitalisation, markdown-emphasis and missing-space tolerance while making it
impossible to cross a newline. Two regression tests added, one per affected
field.
Versioning: eval.yaml was already at 3-B on main, bumped by UKGovernmentBEIS#2230 for a change
documented as leaving results unchanged. This one does change verdicts and
confidences, so it now bumps to 4-B with its own README section and the
changelog fragment relabelled, rather than riding on someone else's version.
That also resolves the self-contradictory [3-B] section.
eval.yaml was already at 5-B on main, bumped by UKGovernmentBEIS#2230 for a change explicitly documented as 'no existing result changes and no re-run is needed'. This PR does change scores on affected samples, so it was shipping a comparability break under a version number that says results are unchanged, and the [5-B] README section carried two bullets contradicting each other on the only question that section answers. Bumps to 6-B with its own README section and the changelog fragment relabelled, which also removes the duplicate 'AssistantBench (v5-B)' bullet that would otherwise appear twice in the next release notes. Adds the missing PR link to the bullet, matching the sibling entries.
This PR contains
Description
mean_ofcan report0.0when it measured nothing at all.This came up in review on #2173, where the automated reviewer pointed out that the shared helper violates the convention that PR documents.
REPO_CONTEXT.md:92tells contributors to usemean_ofinstead of reimplementing the mean-over-dict-value pattern, and it has 43 call sites, so it seemed worth fixing rather than only documenting.The reachable one
With
on_missing="skip", a sample missing the key doesn't incrementcount. If no sample carries the key,countstays 0 and the final line returns0.0:That's reachable in an otherwise healthy run. The samples scored fine, they just don't carry this key. Reproduced against
inspect_ai0.3.259 with three normally scored samples:0.0is a real value on every scale these metrics use, so from the outside an all-miss run and an unmeasured one look identical. Seven call sites useskiptoday:assistant_bench,browse_comp,cyberseceval_4/instruct_or_autocomplete,mind2web(x2) andtac(x2).The unreachable one
if not scores: return 0.0at the top is dead from the framework, which doesn't invoke a metric when every sample is unscored. I removed it rather than changing it, because the loop already handles an empty list:countstays 0 and falls through to the same return. Direct callers getnantoo, which matters because unit tests and metrics that delegate to this helper both invoke it directly.What I changed
Both paths now return
nan, matching whatagentharm'savg_score_non_refusalsdoes after #2174, and what #2179 proposes forpercentage_ofincyberseceval_4.Tests: three new ones that fail on main (all-skipped with real samples, all-None-and-skipped, empty list), plus two that pass on main and must keep passing, so this doesn't just turn every zero into
nan. A measured0.0is still0.0, andon_missing="zero"still counts missing keys as measured zeros.Call sites I checked
All seven
skipsites return the metric straight to the framework, none does arithmetic on the result, sonanflows the same wayagentharm's already does.Two existing tests asserted the old behaviour and now assert
nan:TestAssistantBenchAccuracy::test_empty_scoresandTestAnswerRate::test_empty_scores, both on the empty-list path. Flagging that explicitly since it's a behaviour change someone pinned deliberately.Ran
tests/utils,tests/assistant_bench,tests/browse_comp,tests/mind2web,tests/tac,tests/cyberseceval_4andtests/cti_realm: 504 passed, 29 skipped (HF_TOKEN and slow markers), 5 failed.Those 5 are all pre-existing on my machine and none is in the changed area. Three are
tests/utils/test_eval_params.pyfailing onNo module named 'toml', and two are registry-prefix assertions (test_mind2web_task_creationandtest_mitre_tool_definition_matches_expected_contract, both'mind2web'vs'inspect_evals/mind2web'in shape). I checked outmainand reran the same five there: identical failures, identical messages. Calling that out rather than quietly reporting only the green subset.Version bumps
Added, per @MattFisher's call in review and the precedent settled on #2188: bump
Nwhere reported numbers can move. The five evals holding the sevenskipsites are bumped, with a one-line README changelog entry each.4-B→5-Bassistant_bench_accuracy2-B→3-Bbrowse_comp_accuracy4-B→5-Bbleu_score_average2-A→3-Aelement_accuracy,action_f16-C→7-Cwelfare_rate,completion_rateThe remaining call sites are untouched. They pass
on_missing="error"or"zero", so their behaviour changes only on the empty-scores path the framework does not invoke.cyberseceval_4/eval.yamlwill collide with #2179, which bumps the same file. Whichever lands second rebumps.Also noting #2122 plans to migrate these helpers to
aggregate(). This is a small fix in the meantime and shouldn't complicate that.Checklist
Does this change affect existing eval(s)? If yes:
assistant_bench4-B→5-B,browse_comp2-B→3-B,cyberseceval_44-B→5-B,mind2web2-A→3-A,tac6-C→7-C.Yes, indirectly: seven call sites across
assistant_bench,browse_comp,cyberseceval_4/instruct_or_autocomplete,mind2webandtacpasson_missing="skip"and so can reach the changed path. No task definition, dataset, prompt or scoring logic changes, and the reported number only differs where nothing was measured.Is this change consequential to users? If yes:
uv run scriv createbeen run and the changelog fragment committed? See Fragment Format.changelog.d/20260821_mean_of_empty_denominator.md. The five bumped evals are named underExisting Evalswith their new versions; the shared-utility entry formean_ofitself stays underOther. It is filed there rather than underShared Utilities, which is not one of the three categories declared inpyproject.tomland so would have been dropped from the release notes.Consequential in a narrow way: anyone parsing these metrics gets
nanwhere they used to get0.0, but only on runs where the0.0didn't mean anything. Worth saying out loud because a downstream reader doing arithmetic on the result will now propagatenaninstead of silently averaging in a zero, which is the point.Does this change affect how future contributors write or submit evaluations (e.g. new required fields, changed tooling, updated conventions)? If yes:
No.
mean_ofkeeps its signature and its threeon_missingmodes, so nothing changes about how a new eval calls it.