Skip to content

Add normalized correction metrics to the inference aggregators#1347

Open
jpdunc23 wants to merge 13 commits into
mainfrom
feature/correction-delta-metrics
Open

Add normalized correction metrics to the inference aggregators#1347
jpdunc23 wants to merge 13 commits into
mainfrom
feature/correction-delta-metrics

Conversation

@jpdunc23

@jpdunc23 jpdunc23 commented Jul 7, 2026

Copy link
Copy Markdown
Member

Corrector-equipped runs currently write their per-step correction deltas to autoregressive_step_diagnostics.nc but log no metrics about them, so there is no at-a-glance answer to "how much does this model lean on its corrector?". This PR adds normalized-space correction metrics to both inference aggregators, computed from the StepDiagnostics attached to prediction data: an area-weighted global mean of the time-mean |normalized correction| per corrected variable (plus an opt-in signed time-mean map image) under the existing time_mean_norm group, and per-forecast-step area-weighted magnitude/std series under mean_norm. The normalized correction is normalize(delta, apply_mean=False) — i.e. delta / std, using the apply_mean option from #1353, since mean-centering a difference quantity would wrongly subtract the per-variable mean offset; all reductions go through the mask-aware GriddedOperations, so NaN off-mask cells drop out of the scalars. Metrics are uniform over all corrected variables and silent when no corrector ran (or in coupled inference, which does not attach step diagnostics). Inline training keeps the time-mean metrics but drops the per-step series, matching the existing time-series behavior.

The wiring mirrors the StepDiagnostics container rather than its current contents: each host aggregator carries a single optional StepDiagnosticsAggregator — a concrete wrapper owning a dict of per-concern sub-aggregators (one entry today — the correction-delta aggregator) and fanning record/log/flush out over it — so future StepDiagnostics fields get sibling entries inside the wrapper instead of more host surgery. Granular sub-aggregators for specific container fields (just delta, for now) live another level down and are equally invisible to the hosts.

Changes:

  • fme.ace.aggregator.inference.step_diagnostics (new): the StepDiagnosticsAggregator wrapper, the StepDiagnosticsSubAggregator protocol, StepDiagnosticsMetricConfig, the CorrectionDeltaAggregator, and its granular CorrectionDeltaTimeMeanAggregator / CorrectionDeltaMeanAggregator sub-aggregators

  • InferenceEvaluatorAggregatorConfig.step_diagnostics and InferenceAggregatorConfig.step_diagnostics: new StepDiagnosticsMetricConfig fields controlling metric granularity — correction_scalars (default on) and correction_maps (default off)

  • Both inference aggregators gain step_diagnostics_aggregator: StepDiagnosticsAggregator | None = None and delegate record/log/flush to it

  • InferenceAggregatorConfig.build gains an optional normalize argument (default None); the no-target inference entrypoint now passes stepper.normalizer.normalize. With no normalizer, a default step_diagnostics config silently skips the correction metrics (preserving current behavior for callers that omit normalize, e.g. the coupled builder), while an explicitly non-default config raises rather than silently dropping an opt-in

  • reduced.py's series-assembly dataclass is public (SeriesData), and CorrectionDeltaTimeMeanAggregator raises if a batch changes the sample count or variable set mid-accumulation instead of silently biasing the time-means

  • Tests added

  • If dependencies changed, "deps only" image rebuilt and "latest_deps_only_image.txt" file updated

Corrector-equipped runs currently write their per-step correction deltas to autoregressive_step_diagnostics.nc but log no metrics about them, so there is no at-a-glance answer to "how much does this model lean on its corrector?". This PR adds normalized-space correction metrics to both inference aggregators, computed from the StepDiagnostics attached to prediction data: an area-weighted global mean of the time-mean |normalized correction| per corrected variable (plus a channel mean and a signed time-mean map image) under the existing time_mean_norm group, and per-forecast-step area-weighted magnitude/std series under mean_norm. The normalized correction is computed as normalize(prediction) - normalize(prediction - delta), since normalizing the raw delta would wrongly subtract the per-variable mean offset; all reductions go through the mask-aware GriddedOperations, so NaN off-mask cells drop out of the scalars. Metrics are uniform over all corrected variables, on by default via log_correction_metrics flags, and silent when no corrector ran (or in coupled inference, which does not attach step diagnostics). Inline training keeps the time-mean metrics but drops the per-step series, matching the existing time-series behavior.
Comment thread pr-plan.md Outdated
Comment thread pr-plan.md Outdated
Comment thread pr-plan.md Outdated
Comment thread pr-plan.md Outdated
Comment thread pr-plan.md Outdated
Comment thread pr-plan.md Outdated
…aggregator surface

Review round 1: hosts now carry a generic dict of StepDiagnosticsAggregators
instead of a delta-specific recorder; config becomes StepDiagnosticsMetricConfig
with scalars on / maps off by default; channel_mean dropped; the
normalize(pred) - normalize(pred - delta) formulation is documented as the
uncentered delta/std (no center=False normalizer option needed).
Comment thread pr-plan.md Outdated
Comment thread pr-plan.md Outdated
jpdunc23 added 6 commits July 9, 2026 09:15
- Hosts hold a single optional StepDiagnosticsAggregator (concrete
  wrapper owning the dict of per-concern sub-aggregators, which satisfy
  a StepDiagnosticsSubAggregator protocol) instead of dict fan-out in
  the host aggregators.
- Rename compute_correction_norm -> normalize_delta_without_mean.
Update pr-plan.md for the merged apply_mean option (#1353): compute the normalized correction directly as normalize(delta, apply_mean=False) instead of a normalize_delta_without_mean helper, and use the NormalizeFn protocol in normalize-passing signatures.
Corrector-equipped runs now log how much the model leans on its
corrector: normalized-space correction-magnitude metrics computed from
the StepDiagnostics delta carried on prediction data, merged into the
existing time_mean_norm / mean_norm metric groups in both the evaluator
and no-target inference aggregators. Each host carries a single optional
StepDiagnosticsAggregator that owns per-concern sub-aggregators (the
correction delta today) and fans record/log/flush calls out over them.
Scalar metrics are on by default and map images are opt-in via the new
step_diagnostics config on both aggregator configs; runs without a
corrector (or without a normalizer, for the no-target aggregator) are
unaffected.
@jpdunc23

jpdunc23 commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Pre-review agent: I ran a pre-review audit of this PR ahead of human review, in a clean session separate from the one that built the change. All 19 new tests pass locally and ruff/mypy are clean. Three observations for the reviewer, none blocking:

  1. Silent skip when no normalizer is supplied. StepDiagnosticsMetricConfig.build returns None whenever normalize is None, including when the user explicitly enabled correction_scalars/correction_maps. Both in-repo entrypoints now pass a normalizer, and the behavior is documented in the config docstring, but an explicit opt-in being silently dropped (rather than raising) is a deliberate trade-off the reviewer may want to sign off on.
  2. Unguarded accumulation invariants in CorrectionDeltaTimeMeanAggregator. record_batch overwrites self._n_samples on every call and divides each variable's accumulated sum by the global timestep count, which assumes a constant sample count and a constant variable set across windows. The current inference loop guarantees both, but nothing asserts them — a future caller violating either would get silently biased time-means rather than an error.
  3. Multi-window accumulation is untested. No test records two batches into the time-mean aggregator, so the accumulation arithmetic in (2) is exercised only for the single-window case. A two-call test with a hand-computed expected mean would lock it down.

Minor: step_diagnostics.py imports the private _SeriesData from reduced.py — the only cross-module use of that private name; consider making it public.

— 🤖 Drafted by Claude (Fable 5) via Claude Code, posted on @jpdunc23's behalf.

@jpdunc23

jpdunc23 commented Jul 9, 2026

Copy link
Copy Markdown
Member Author
  1. Silent skip when no normalizer is supplied. StepDiagnosticsMetricConfig.build returns None whenever normalize is None, including when the user explicitly enabled correction_scalars/correction_maps. Both in-repo entrypoints now pass a normalizer, and the behavior is documented in the config docstring, but an explicit opt-in being silently dropped (rather than raising) is a deliberate trade-off the reviewer may want to sign off on.

I think we already have precedents of raising errors in the aggregators when user config and build-time data availability are mismatched. Should we do the same here?

  1. Unguarded accumulation invariants in CorrectionDeltaTimeMeanAggregator. record_batch overwrites self._n_samples on every call and divides each variable's accumulated sum by the global timestep count, which assumes a constant sample count and a constant variable set across windows. The current inference loop guarantees both, but nothing asserts them — a future caller violating either would get silently biased time-means rather than an error.

Is this a preexisting issue in other time mean aggregators? If not, we should follow their prior art. If so, do we just need to provide some better documentation of the record_batch assumptions?

  1. Multi-window accumulation is untested. No test records two batches into the time-mean aggregator, so the accumulation arithmetic in (2) is exercised only for the single-window case. A two-call test with a hand-computed expected mean would lock it down.

Let's add this test.

Minor: step_diagnostics.py imports the private _SeriesData from reduced.py — the only cross-module use of that private name; consider making it public.

Let's make it public.

Comment thread fme/ace/aggregator/inference/step_diagnostics.py Outdated
Comment thread pr-plan.md Outdated
Pull MeanAggregator._get_series_data and its get_dataset body out into
public get_series_data / series_data_to_dataset helpers in reduced.py,
reused by MeanAggregator, SingleTargetMeanAggregator, and
CorrectionDeltaMeanAggregator. SingleTargetMeanAggregator.get_dataset
now returns an empty dataset instead of raising when nothing was
recorded, inheriting the empty-safe branch.

Remove the pr-plan.md planning artifact ahead of merge.
Comment thread fme/ace/aggregator/inference/step_diagnostics.py Outdated
Comment thread fme/ace/aggregator/inference/step_diagnostics.py Outdated
Comment thread fme/ace/aggregator/inference/step_diagnostics.py Outdated
Comment thread fme/ace/aggregator/inference/step_diagnostics.py
…on invariants, public SeriesData

- StepDiagnosticsMetricConfig.build raises ValueError when the config is
  explicitly non-default but no normalizer is supplied; the default config
  keeps the silent skip (the coupled builder constructs
  InferenceAggregatorConfig with defaults and no normalizer).
- CorrectionDeltaTimeMeanAggregator.record_batch documents its
  contiguous-windows assumptions and raises ValueError if a batch changes
  the sample count or variable set mid-accumulation.
- Add a two-window accumulation test with a hand-computed mean, plus tests
  for the new errors.
- Rename reduced.py's _SeriesData to public SeriesData.
@jpdunc23

Copy link
Copy Markdown
Member Author

All four addressed in cd6dd05:

  1. Followed the existing precedent, with one nuance: StepDiagnosticsMetricConfig.build now raises ValueError when the configuration was explicitly changed from the defaults but no normalizer is supplied. The default configuration (scalars on) keeps the silent skip — raising unconditionally would break the coupled inference aggregator builder, which constructs the per-component InferenceAggregatorConfig with defaults and no normalizer.
  2. Partially preexisting: TimeMeanAggregator sets its sample count once rather than overwriting it, and a changed variable set there fails with a KeyError rather than biasing. I brought the correction time-mean aggregator up to (slightly beyond) that bar: record_batch documents its contiguous-windows assumptions and raises ValueError if a batch changes the sample count or the variable set mid-accumulation.
  3. Added a two-window accumulation test with a hand-computed expected mean over unequal window lengths, plus tests for the two new invariant errors and the build-time error.
  4. Renamed to public SeriesData.

— 🤖 Drafted by Claude (Fable 5) via Claude Code, posted on @jpdunc23's behalf.

jpdunc23 and others added 2 commits July 9, 2026 20:21
The prediction was only needed for normalization before #1353; the
record-time time-alignment check goes with it (the delta's alignment is
guaranteed by StepOutput.stack_diagnostics's construction). Also remove
the coupled-inference example from a comment that would go stale when
coupled carriage lands.
@jpdunc23 jpdunc23 marked this pull request as ready for review July 10, 2026 16:40

@mcgibbon mcgibbon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only optional comments.

Comment on lines +28 to +48
class StepDiagnosticsSubAggregator(Protocol):
"""A per-concern aggregator of metrics from the StepDiagnostics carried
on prediction data. Entries in the StepDiagnosticsAggregator's dict
satisfy this; the wrapper delegates record/log/flush calls without
knowing what any entry measures.
"""

@property
def log_time_series(self) -> bool: ...

def record_batch(
self,
step_diagnostics: StepDiagnostics | None,
i_time_start: int,
) -> None: ...

def summary_logs(self) -> dict[str, Any]: ...

def time_series_logs(self, step_slice: slice) -> dict[str, Any]: ...

def diagnostics(self) -> dict[str, xr.Dataset]: ...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (optional): Use StepDiagnosticsSubAggregator = CorrectionDeltaAggregator (or whatever the correct form is for type variable assignment, and update this to equal the union of types when we have more than one such example. Or I don't mind you leaving it as-is, especially if this is replicating an existing pattern.

Optional: If you want a stronger guarantee that new implementations have the API, you could use an ABC instead of protocol.

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.

2 participants