Skip to content

Aggregate: ARGMIN/ARGMAX return coordinates; fix RangedAggregate async passthrough - #199

Open
cboulay wants to merge 1 commit into
devfrom
fix/aggregate-argminmax-coordinates
Open

Aggregate: ARGMIN/ARGMAX return coordinates; fix RangedAggregate async passthrough#199
cboulay wants to merge 1 commit into
devfrom
fix/aggregate-argminmax-coordinates

Conversation

@cboulay

@cboulay cboulay commented Aug 7, 2026

Copy link
Copy Markdown
Member

Closes #192.

AggregateTransformer ARG* returns coordinates

AggregateTransformer returned ARGMIN/ARGMAX as a raw index into the aggregated axis, while RangedAggregateTransformer and BinnedAggregateTransformer return the axis coordinate for the same enum member. It is worse here than for the other two, because this transformer removes the axis from the output — the consumer is handed an index into a dimension the message no longer describes, and cannot convert it without having kept the input around.

The machinery was already in place; AggregateTransformer was opting out of it. This drops index_to_coordinate=False and widens the coordinate condition from op == TRAPEZOID to the existing needs_coordinates(op), which already covers both ARG* members.

d = np.zeros((1, 10)); d[0, 7] = 1.0     # peak at index 7 -> 14.0 Hz
msg = AxisArray(data=d, dims=["ch", "freq"],
                axes={"freq": AxisArray.LinearAxis(gain=2.0, offset=0.0, unit="Hz")}, key="k")

AggregateTransformer(AggregateSettings(axis="freq", operation=A.ARGMAX))(msg).data
# before: array([7])     <- index, and "freq" is gone from dims
# after:  array([14.])   <- Hz

Breaking. ARG* output units change from index to axis coordinate. TRAPEZOID was already correct and is unaffected, and no in-repo caller uses ARG* through either transformer (fbcca.py calls numpy.argmin directly). Where the axis carries no metadata, get_axis supplies a default LinearAxis (gain=1, offset=0), so the coordinates are 0, 1, 2, … and the result is the index after all — covered by a test.

index_to_coordinate stays on aggregate_slices for a caller that means to index back into the array it passed in; nothing in this package sets it False any more, and the docstring now says so instead of naming AggregateTransformer.

Separate bug: RangedAggregate(bands=None) was broken in every graph

Found while reading. RangedAggregateTransformer overrode __call__ for its bands is None passthrough but not __acall__. BaseTransformerUnit.on_signal awaits __acall__, so in a graph the shortcut was skipped entirely and bands=None fell through to _reset_state:

TypeError: 'NoneType' object is not iterable    # for start, stop in self.settings.bands

The shortcut only ever worked when the transformer was driven synchronously, which is why every existing test passed. BinnedAggregateTransformer overrides both.

Both branches now also call _request_reset(), matching BinnedAggregate, so toggling bands to None and back to a different list cannot resume on the first list's slices.

Tests

  • test_aggregate_transformer_argminmax updated to expect coordinates (freq is gain=2.0/offset=1.0, so index and coordinate genuinely differ).
  • test_aggregate_argminmax_matches_ranged — new, pins the actual invariant: a band spanning the whole axis asks the same question as the full-axis reduction, so the two transformers must agree. Guards against a future re-split.
  • test_aggregate_argminmax_bare_axis_is_index — new, the no-metadata fallback.
  • test_ranged_aggregate_passthrough_async / ..._resume_async — new, cover the __acall__ path.

Full suite: 3699 passed, 5 skipped. Ruff clean.

AggregateTransformer returned ARGMIN/ARGMAX as a raw index into the axis
it then removes from the message, so the consumer was handed an index into
a dimension the output no longer describes. RangedAggregate and
BinnedAggregate answer the same enum member with the axis coordinate.

Drop `index_to_coordinate=False` and widen the coordinate condition from
`op == TRAPEZOID` to the existing `needs_coordinates(op)`, which already
covers both ARG* members. Where the axis carries no metadata `get_axis`
supplies a default LinearAxis (gain=1, offset=0), so the coordinates are
0, 1, 2, ... and the result is the index after all.

BREAKING: ARG* output on AggregateTransformer changes from an index to an
axis coordinate. TRAPEZOID was already correct and is unaffected.

Also fixes a separate bug found while reading: RangedAggregateTransformer
overrode __call__ for its `bands is None` passthrough but not __acall__.
BaseTransformerUnit awaits __acall__, so `bands=None` inside a graph fell
through to _reset_state and raised TypeError on iterating None -- the
shortcut only ever worked when the transformer was driven synchronously.
Both branches now also _request_reset(), matching BinnedAggregate, so
resuming with different bands cannot reuse the old slices.

Closes #192
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.

AggregateTransformer ARGMIN/ARGMAX return a raw index into an axis it then drops

1 participant