Add aggregate(key, agg=...) metric factory (#3735) by antnewman · Pull Request #3850 · UKGovernmentBEIS/inspect_ai

antnewman · 2026-05-06T12:58:55Z

Closes #3735.

Implements the aggregate(key, agg) metric factory proposed in the issue and the prior Slack discussion. Many scorers emit dict-valued Score.value (multiple numeric fields per sample); aggregate extracts one field by key and applies any standard metric (mean, stderr, std, accuracy, ...) to it, replacing the need for bespoke per-aggregator factories like mean_of / std_of / stderr_of in downstream evals.

Usage

from inspect_ai.scorer import aggregate, mean, stderr

@metric
def element_accuracy() -> Metric:
    return aggregate("element_acc", agg=mean())

@metric
def element_accuracy_stderr() -> Metric:
    return aggregate("element_acc", agg=stderr())

Behaviour

on_missing="error" (default) raises ValueError if any sample lacks the key.
on_missing="skip" excludes such samples from the inner aggregator.
on_missing="zero" substitutes 0.0.
A non-dict Score.value always raises ValueError with a message naming the offending sample (matches grouped()'s strictness for type errors).
Sample identity (sample_id, sample_metadata, scorer) is preserved when constructing the inner SampleScores, so metrics that consult metadata (e.g. clustered stderr) continue to work.

Scope

This PR adds aggregate() only — grouped() is not refactored to use it, to keep the diff focused. Happy to follow up with that if maintainers prefer.

This PR contains:

What is the current behavior?

No general-purpose primitive for applying a metric to a single field of dict-valued scores. inspect_evals ships a mean_of helper, but extending to other aggregators requires a new factory each time (see cyberseceval_4's std_of / stderr_of / percentage_of / count_of).

What is the new behavior?

aggregate(key, agg, *, to_float=value_to_float(), on_missing="error") returns a Metric that applies agg to the key field extracted from each sample.

Does this PR introduce a breaking change?

No. New public function exported from inspect_ai.scorer.

Other information

Tests cover composition with mean() and stderr(), on_missing modes, key-selection across multiple keys, non-dict-value error path, and to_float application for "C"/"I" values. Docs added under "Per-Key Aggregation" in docs/scorers.qmd.

…tBEIS#3735) Many scorers emit dict-valued Score.value (multiple numeric fields per sample) and then need per-key metrics. `aggregate(key, agg)` selects one field by key and applies any standard metric (mean, stderr, accuracy, ...) to it, replacing the need for bespoke per-aggregator factories like mean_of/std_of/stderr_of. - on_missing controls samples missing the key: "error" (default), "skip", "zero". - to_float follows the same convention as the other built-in metrics. - Non-dict Score.value raises ValueError with a clear message. Includes tests covering composition with mean/stderr, on_missing modes, and key-selection semantics. Doc section added under "Per-Key Aggregation" in docs/scorers.qmd.

WatchTree-19 · 2026-05-08T13:45:48Z

two non-blocking observations:

value[key] being None (key present but value is None) hits to_float(None) directly rather than going through the on_missing branch. value_to_float() likely raises on None (it falls through to the float(...) cast). probably edge enough to defer, but worth a sentence in the docstring noting that None values are passed straight through to to_float, not treated as missing.
on_missing="skip" with agg=stderr() quietly changes the stderr's denominator. two scorers running the same eval, one where all samples have key and one where half do, will report different stderrs purely from skip semantics, not from underlying variance. worth flagging in the docstring; no code change needed.

lgtm otherwise. clean separation of concerns, sample identity (sample_id, sample_metadata, scorer) preserved through model_copy, and errors include sample_id which is the right call.

antnewman · 2026-05-08T13:57:32Z

Thanks @WatchTree-19, both fair points. Pushing a docstring update.

Small nuance on (1): value_to_float() doesn't actually raise on None — it falls through to a logger.warning and returns 0.0. Functionally close to on_missing="zero" but with a warning, which is arguably the worst of both worlds when the user wanted "skip". I'll document the present-but-None behaviour explicitly so the contract is clear, and we can decide separately whether to treat None as missing in a follow-up.

On (2), good catch — added a "Notes" paragraph flagging that skip changes the inner aggregator's sample count, with stderr as the worked example.

…tics Address review feedback on UKGovernmentBEIS#3850: - Note that `on_missing="skip"` changes the sample count seen by the inner aggregator, so denominator-sensitive metrics (`stderr`, `mean`, `std`, `var`) can report different values purely from differing missing-key rates rather than underlying variance. - Document that a present-but-`None` key is not treated as missing; it is passed through to `to_float`, which with the default `value_to_float()` warns and returns `0.0`. No behaviour change.

WatchTree-19 · 2026-05-10T13:33:55Z

ah, good catch on the warn-then-zero behaviour - that's worse than raising in this context because the eval still completes with a quietly-wrong number rather than failing loud. agreed that treating None-as-missing is a separate decision; the docstring note is the right scope for this PR.

thanks for both updates. lgtm.

antnewman · 2026-05-10T13:40:42Z

Thanks for the careful read @WatchTree-19 — the warn-then-zero framing is a much sharper way of putting it than I had. Appreciated.

ppcvote

Drive-by review from #3919 (now closed as duplicate of this).

Two things I'd add for the inspect_evals.utils.mean_of migration angle — they're docstring-level, not implementation:

1. Semantic-change call-out for None-valued keys. mean_of currently routes a present-but-None value through on_missing (i.e. it's treated as missing). This PR's aggregate() passes None straight through to_float, which under the default value_to_float() falls through to a 0.0-with-warning. The semantics are arguably cleaner here, but it's a quiet behavior change for anyone doing a straight mean_of("k") → aggregate("k", agg=mean()) swap. One additional sentence in the docstring (or a migration note in the CHANGELOG) saying "if you're migrating from mean_of, note that present-but-None values are handled differently — pass them through your own to_float if you want the old behavior" would close that gap.

2. on_missing="skip" denominator subtlety. The docstring already flags that skip reduces the sample count seen by agg, which changes stderr / mean / std. Great call-out. The complementary case worth mentioning is: when a downstream consumer compares two evals using the same aggregate(..., on_missing="skip") metric across the same dataset, they should be aware that different rates of missing keys produce different stderrs even when the underlying signal is identical. A pointer to "zero" as the constant-denominator alternative — which the prose already hints at — could be made explicit ("for cross-eval comparability, prefer 'zero'"). Tiny nit.

The shape of the factory itself is what I'd have written, including the model_copy(update=...) for the inner sample rebuild — that's cleaner than the field-by-field reconstruction I tried first. Composition with stderr() / std() is the thing I'd most want to see explicit tests for; if you'd like I can prep a small follow-up that adds those (and the mean_of parity case) once this lands.

@ppcvote

Address @ppcvote's suggestion on UKGovernmentBEIS#3735: surface the semantic difference vs `inspect_evals.utils.mean_of` (which treats present-but- None as missing) so a straight swap doesn't silently change behaviour. No code change.

dragonstyle

I'd like us to properly handle present but None by routing that through on_missing. This is the correct behavior and will bring things in line with the current mean_of behavior.
If skip filters all values (so there are no values to aggregate), should we just throw here or return Nan?

@dragonstyle

… NaN on all-skip Address @dragonstyle's review of UKGovernmentBEIS#3850: - Present-but-None values are now routed through `on_missing` (matching inspect_evals.utils.metrics.mean_of). Previously they fell through to to_float(None), which under the default value_to_float() warns and returns 0.0 — silently conflating "no signal" with "scored 0". - When `on_missing="skip"` filters every sample, aggregate() now returns NaN rather than calling agg([]) (which most built-in metrics raise on). Mirrors the Score.unscored() / NaN sentinel used elsewhere in the framework. Adds 4 tests covering the None-handling on each on_missing mode and the all-skipped → NaN case. CHANGELOG updated to remove the now-obsolete migration note about the semantic difference vs mean_of (which no longer exists).

antnewman · 2026-05-31T08:42:16Z

Both addressed in 3e44527f8:

Present-but-None now routed through on_missing, matching mean_of. Previously a None value fell through to to_float(None), which under the default value_to_float() warns and returns 0.0 — silently conflating "no signal" with "scored 0". Now error raises, skip excludes, zero substitutes 0.0, all the same as a missing key. Added three tests covering the three modes.
skip filtering every sample returns NaN rather than calling agg([]) (which most built-in metrics raise on). Mirrors the Score.unscored() / NaN sentinel used elsewhere — preserves the "no measurable value" signal without crashing aggregators downstream. Added a test for it.

CHANGELOG updated to drop the now-obsolete migration note (the semantic difference vs mean_of no longer exists).

dragonstyle · 2026-05-27T14:02:30Z

+    """
+
+    def aggregate_metric(scores: list[SampleScore]) -> Value:
+        agg_protocol = cast(MetricProtocol, agg)


Is this cast needed?

Yes — Metric is MetricProtocol | MetricDeprecated, and MetricDeprecated.__call__ takes list[Score] while we're passing list[SampleScore], so mypy needs the narrowing to the protocol form to accept agg(extracted). It mirrors the same cast(MetricProtocol, metric) in grouped() (_metrics/grouped.py), which wraps an inner metric the same way. Happy to restructure if you'd prefer to avoid the cast, but this keeps it consistent with the existing pattern.

dragonstyle · 2026-06-01T23:30:00Z

Code reporting these three issues:

P1 aggregate.py (line 91): per-key NaN values are treated as scored values and passed into the inner metric. Existing dict-metric expansion skips {"x": NaN} for key x and counts it unscored; this returns NaN for the whole aggregate and reports all samples as scored. I reproduced [{x: 1}, {x: NaN}, {x: 3}]: existing dict metrics return 2.0 with 2 scored / 1 unscored, while aggregate("x", mean()) returns NaN with 3 scored / 0 unscored.

P2 aggregate.py (line 92): the wrapper converts extracted values before calling agg, so custom converters on the inner metric are silently bypassed. Example: aggregate("v", agg=accuracy(to_float=value_to_float(correct="pass", incorrect="fail"))) returns 0.0 for ["pass", "fail"]; passing the converter to aggregate(..., to_float=...) returns 0.5. That is surprising given normal metric docs teach users to configure accuracy(to_float=...).

P3 aggregate.py (line 102): invalid on_missing values silently behave like "zero". A typo such as on_missing="skpi" produces a numeric result instead of failing, but only when a key is missing, which makes this easy to miss.

Address two of dragonstyle's review findings on UKGovernmentBEIS#3850: - P1: a per-key NaN value (e.g. {"x": NaN}) was converted and passed to the inner metric as a scored value, dragging the whole aggregate to NaN. The framework's dict-metric expansion instead skips NaN-valued keys as unscored. aggregate() now does the same — NaN values are skipped regardless of on_missing (which governs *missing keys*, a distinct concept). aggregate("x", mean()) over [{x:1},{x:NaN},{x:3}] now returns 2.0 instead of NaN. - P3: an invalid on_missing (e.g. "skpi") silently behaved like "zero", and only when a key happened to be missing. It's now validated at construction time and raises ValueError immediately. The third finding (P2 — aggregate's to_float bypassing a custom converter on the inner metric) is a deliberate API-design choice left for the maintainer to direct; raised separately in a PR comment. Adds tests: NaN skipped (default + on_missing="zero"), all-NaN → NaN, invalid on_missing raises.

antnewman · 2026-06-02T08:17:58Z

Thanks for the careful review @dragonstyle. P1 and P3 are fixed in 2c3233e35; P2 needs a design call from you before I touch it.

P1 — per-key NaN treated as scored. Fixed. A per-key NaN is now skipped as unscored regardless of on_missing (which governs missing keys — a distinct concept), matching the dict-metric expansion in results.py. aggregate("x", mean()) over [{x:1},{x:NaN},{x:3}] now returns 2.0. One honest caveat: the value is now correct, but the scored/unscored count you quoted (2 scored / 1 unscored) is computed in results.py from the root score values, which for aggregate are dicts, not NaN-floats — so the per-key count isn't something this metric can set on its own. If you'd like the count to reflect per-key NaN too, that's a results.py-layer change I'm happy to look at separately. Tests added for NaN-skip (default + zero), all-NaN → NaN.

P3 — invalid on_missing silently acts like zero. Fixed. Now validated at construction time and raises ValueError immediately, regardless of whether a key is ever missing. Test added.

P2 — inner metric's to_float bypassed. Real, but it's a genuine API choice I'd rather you steer, because to_float is load-bearing in a way that isn't obvious: mean()/std()/stderr() use score.as_float(), which raises on non-numeric strings like "C". Only accuracy() (and metrics taking to_float) self-convert. So aggregate's to_float is what lets "C"/"I" strings flow into mean(). Two ways to resolve your finding:

Option A — passthrough default. to_float: ValueToFloat | None = None. When None (new default), pass the raw extracted value straight through so the inner metric's own converter wins (fixes your accuracy(to_float=...) example); when explicitly set, aggregate converts (escape hatch for feeding strings into mean()). Smallest behaviour change, most capable.
Option B — remove to_float entirely. aggregate becomes pure extraction; the inner metric always converts. Simplest and most opinionated; aggregate("k", mean()) over "C"/"I" would then need accuracy() instead.

I lean A, but it's your API — which would you prefer? Happy to push it straight after.

Re the inline question on the cast(MetricProtocol, agg) — yes, it's needed: Metric is the MetricProtocol | MetricDeprecated union, and calling agg(scores) requires the protocol form. Happy to restructure if you'd rather avoid the cast.

dragonstyle · 2026-06-02T13:10:22Z

P2 - I agree with option A - I think defaulting to None and allowing the metric to_float in this case makes sense.

@dragonstyle

Implements @dragonstyle's chosen Option A on UKGovernmentBEIS#3850. Previously aggregate always pre-converted extracted values with its own `to_float` (`value_to_float()` by default), silently bypassing any custom converter on the inner metric — e.g. accuracy(to_float=value_to_float(correct="pass")) never saw "pass"/"fail" because aggregate had already mapped them to 0.0. `to_float` now defaults to None: the raw extracted value passes straight through to `agg`, so the inner metric's own conversion applies (accuracy()'s to_float, mean()'s as_float). An explicit `to_float=` is the escape hatch for feeding non-numeric values (e.g. "C"/"I" string grades) into metrics like mean() that can't convert them. - test_aggregate_to_float_applied → test_aggregate_explicit_to_float_applied (now passes to_float=value_to_float() explicitly). - Add test_aggregate_passthrough_respects_inner_to_float, asserting a custom value_to_float on accuracy() is honoured (the exact case in the review).

antnewman · 2026-06-02T13:18:46Z

Option A implemented in de654bb63. to_float now defaults to None: the raw extracted value passes straight through to agg, so the inner metric's own converter applies. The accuracy(to_float=value_to_float(correct="pass", incorrect="fail")) case from your finding now works — added test_aggregate_passthrough_respects_inner_to_float asserting exactly that. The explicit to_float= escape hatch (for feeding string grades into mean(), which can't self-convert) is covered by test_aggregate_explicit_to_float_applied.

That's all three findings addressed. Let me know on the cast — happy to restructure to drop it if you'd prefer, otherwise I'll leave it as the minimal way to satisfy the MetricProtocol | MetricDeprecated union.

antnewman · 2026-06-06T09:56:08Z

Heads-up: CI hasn't kicked off on the latest commit (de654bb63) — looks like the fork-PR workflow needs an "approve and run" release for this push. All three review findings are addressed and the suite is green locally (17/17 aggregate tests, ruff + mypy clean), so it should sail through once the run is authorised. No rush from my side — just flagging in case it's waiting on you.

antnewman · 2026-06-19T15:15:15Z

Friendly check-in @dragonstyle @jjallaire — no rush, just surfacing the current state so nothing's silently stuck:

This PR (Add aggregate(key, agg=...) metric factory (#3735) #3850): all three review findings are addressed (P1 NaN-skip, P3 on_missing validation, P2 passthrough to_float per your Option A pick). The only snag is CI hasn't run on the latest commit de654bb63 — looks like the fork-PR workflow needs an "approve and run" release. Green locally (ruff/mypy/17 aggregate tests).
A few sibling PRs are also sitting green and review-ready whenever there's bandwidth: Atomic log file writes (#2949) #3950 (atomic local log writes — all of @jjallaire's umask/mode rounds done), fix(scorer): tag math() answer-extraction failures with reason metadata (#4026) #4091 (math() → NOANSWER on parse-fail, sister to fix(scorer): mark model_graded grade-parse failure as unscored (not INCORRECT) #4048), and Warn when task_args are entirely unconsumed by any task (#4194) #4224 (warn on unconsumed task_args, closes Warn when task_args are entirely unconsumed by any task #4194).

Happy to rebase any of them if they've drifted. Thanks for the time!

dragonstyle

@antnewman Apologies for the delay - can you please resolve conflicts on this branch?

…etric # Conflicts: # CHANGELOG.md # docs/scorers.qmd # src/inspect_ai/scorer/__init__.py # src/inspect_ai/scorer/_metrics/__init__.py # tests/scorer/test_metric.py

antnewman · 2026-07-07T19:30:59Z

Conflicts resolved, @dragonstyle — merged latest main in. Nothing tricky in the code; the notable bit was that the scorer docs got restructured on main (scorers.qmd slimmed down and metrics moved to docs/metrics.qmd), so I moved the aggregate documentation into a new "Per-Key Aggregation" section in metrics.qmd alongside grouped(), rather than keeping the old inline section. Also confirmed the ts-mono submodule pointer is untouched.

All aggregate tests plus main's new test_metrics_return_zero_for_empty_scores pass together (19), ruff + mypy clean. Should be ready for another look.

@Metric

…ng test The @Metric decorator erases the Literal["error","skip","zero"] constraint on on_missing, so passing "skpi" was never a type error and the # type: ignore[arg-type] was flagged unused by mypy. Remove it.

…etric # Conflicts: # CHANGELOG.md

antnewman · 2026-07-15T15:04:05Z

@dragonstyle Conflicts resolved — sorry for the silence after your request; I'd actually fixed them the next day but neglected to say so here, and the branch drifted conflicting again in the meantime. It's now merged up to today's main (the only conflict each time is the CHANGELOG; the entry is back under ## Unreleased and trimmed to the new one-line house style). 51 metric tests, ruff and mypy all green locally; CI running now.

I'll keep the branch current so it doesn't block you again — whenever you get a chance to re-review, I'd be glad to clear the changes-requested flag.

…etric # Conflicts: # CHANGELOG.md

jjallaire requested a review from dragonstyle May 11, 2026 14:15

Merge branch 'main' into feature/aggregate-metric

3824328

antnewman mentioned this pull request May 12, 2026

Add aggregate(key, agg=...) metric factory for dict-valued Score.value #3735

Open

ppcvote mentioned this pull request May 12, 2026

feat(scorer): add aggregate(key, agg=metric) factory for dict-valued Score.value #3919

Closed

ppcvote reviewed May 12, 2026

View reviewed changes

antnewman mentioned this pull request May 26, 2026

model_graded_qa / model_graded_fact silently default to INCORRECT when the judge's verdict doesn't match the grade regex #4026

Closed

dragonstyle reviewed May 27, 2026

View reviewed changes

dragonstyle reviewed Jun 1, 2026

View reviewed changes

dragonstyle self-assigned this Jul 2, 2026

dragonstyle requested changes Jul 7, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feature/aggregate-m…

3d8ab7e

…etric # Conflicts: # CHANGELOG.md # docs/scorers.qmd # src/inspect_ai/scorer/__init__.py # src/inspect_ai/scorer/_metrics/__init__.py # tests/scorer/test_metric.py

fix(tests): drop now-unused type-ignore on aggregate invalid-on_missi…

151b579

…ng test The @Metric decorator erases the Literal["error","skip","zero"] constraint on on_missing, so passing "skpi" was never a type error and the # type: ignore[arg-type] was flagged unused by mypy. Remove it.

antnewman added 2 commits July 8, 2026 13:50

Merge remote-tracking branch 'upstream/main' into feature/aggregate-m…

f6207b6

…etric # Conflicts: # CHANGELOG.md

Merge remote-tracking branch 'upstream/main' into feature/aggregate-m…

6bee5d4

…etric # Conflicts: # CHANGELOG.md

Merge remote-tracking branch 'upstream/main' into feature/aggregate-m…

00507c4

…etric # Conflicts: # CHANGELOG.md

Uh oh!

Conversation

antnewman commented May 6, 2026

Usage

Behaviour

Scope

This PR contains:

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

WatchTree-19 commented May 8, 2026

Uh oh!

antnewman commented May 8, 2026

Uh oh!

WatchTree-19 commented May 10, 2026

Uh oh!

antnewman commented May 10, 2026

Uh oh!

ppcvote left a comment

Choose a reason for hiding this comment

Uh oh!

dragonstyle left a comment

Choose a reason for hiding this comment

Uh oh!

antnewman commented May 31, 2026

Uh oh!

dragonstyle May 27, 2026

Choose a reason for hiding this comment

Uh oh!

antnewman Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

dragonstyle commented Jun 1, 2026

Uh oh!

antnewman commented Jun 2, 2026

Uh oh!

dragonstyle commented Jun 2, 2026

Uh oh!

antnewman commented Jun 2, 2026

Uh oh!

antnewman commented Jun 6, 2026

Uh oh!

antnewman commented Jun 19, 2026

Uh oh!

dragonstyle left a comment

Choose a reason for hiding this comment

Uh oh!

antnewman commented Jul 7, 2026

Uh oh!

antnewman commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants