From 91fbab8806c989b4a0ae479fc2c904530da592e3 Mon Sep 17 00:00:00 2001 From: Shijie Gu Date: Tue, 1 Sep 2026 22:18:22 -0700 Subject: [PATCH] Re-resolve a Slicer selection when the axis values change SlicerTransformer caches the indices a selection resolves to and rebuilds them only when _hash_message changes, which keys on the message key and the channel count. For a positional selection that is complete: nothing else can affect the answer. For a label, regex or field selection it is not, because those resolve against the coordinate values, and a source can rename, reorder or swap out channels without changing how many it sends -- a device reconfigured mid-session does exactly that. The failure is silent and worse than a stale slice. `new_axis` is cached alongside the indices, so the output keeps announcing the channels the selection originally matched while carrying another channel's samples: selection "Ch7" axis ['Ch5','Ch7','Ch10'] -> reports 'Ch7', data is position 1 = Ch7 axis ['Ch7','Ch5','Ch10'] -> reports 'Ch7', data is position 1 = Ch5 axis ['Ch1','Ch2','Ch3'] -> reports 'Ch7', data is position 1 = Ch2 Nothing raises, and downstream -- including trace labels on a plot -- believes the axis. In the third case the selected label is not on the stream at all and on_empty never gets the chance to say so. Include the coordinate data in the hash when the selection can consult it. _resolves_by_position keeps that off the hot path for selections built only from slice expressions, which cannot depend on the axis; it deliberately looks at the selection string alone, since whether a bare integer resolves positionally is itself decided by the labels. Costs a tobytes() of the ch axis per message on label/regex/field selections (a few KB at 256 channels, against a slice that already copies the data), and an axis rebuilt identically each message still hashes the same, so a source that rebuilds its ch axis per message does not re-resolve. Co-Authored-By: Claude Opus 5 (1M context) --- src/ezmsg/sigproc/slicer.py | 21 ++++++- tests/unit/test_slicer.py | 119 ++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 1 deletion(-) diff --git a/src/ezmsg/sigproc/slicer.py b/src/ezmsg/sigproc/slicer.py index 70592f6..bc8f690 100644 --- a/src/ezmsg/sigproc/slicer.py +++ b/src/ezmsg/sigproc/slicer.py @@ -191,10 +191,29 @@ class SlicerState: class SlicerTransformer(BaseStatefulTransformer[SlicerSettings, AxisArray, AxisArray, SlicerState]): + def _resolves_by_position(self) -> bool: + """True iff the selection can only ever mean array positions. + + Every token is a slice expression, so no part of it is matched against + the axis's coordinate values and the resolved indices cannot depend on + them. + + It is only a small case, because otherwise we will just use a more complicated hash. + """ + if self.settings.field is not None: + return False + return all(":" in token for token in self.settings.selection.split(",") if token.strip()) + def _hash_message(self, message: AxisArray) -> int: axis = self.settings.axis or message.dims[-1] axis_idx = message.get_axis_idx(axis) - return hash((message.key, message.data.shape[axis_idx])) + key = (message.key, message.data.shape[axis_idx]) + if self._resolves_by_position(): + return hash(key) + # new cache includes the coordinate-axis bytes when the selection can resolve + # against them. + data = getattr(message.axes.get(axis), "data", None) + return hash((*key, None if data is None else np.asarray(data).tobytes())) def _selects_positional_int(self, axinfo: AxisArray.CoordinateAxis | None) -> bool: """True iff the selection is a single bare-integer token that parse_slice diff --git a/tests/unit/test_slicer.py b/tests/unit/test_slicer.py index 3a83084..ac71e4e 100644 --- a/tests/unit/test_slicer.py +++ b/tests/unit/test_slicer.py @@ -447,3 +447,122 @@ def test_slicer_order_single_slice_untouched(): def test_slicer_order_invalid(): with pytest.raises(ValueError, match="order"): SlicerTransformer(SlicerSettings(selection="0:2", axis="ch", order="token"))(_make_order_msg()) + + +def _labelled_msg(labels: list[str], key: str = "test_relabel") -> AxisArray: + """One message whose every column carries its own index as its value. + + That is what lets these tests say *which* channel came out, rather than + only what the output axis claims came out -- the distinction the bug turns + on, since the axis is cached alongside the indices. + + ``_labelled_msg(["Ch5", "Ch7", "Ch10"])`` gives dims ``["time", "ch"]``, + a 100 Hz time axis and:: + + .data [[0., 1., 2.], column i carries i, so a slice + [0., 1., 2.], of the data reports the position + [0., 1., 2.], it was taken from + [0., 1., 2.]] + .axes["ch"].data ["Ch5", "Ch7", "Ch10"] + .key "test_relabel" constant, so the base hash sees + the same stream every time + + Calling it again with ``["Ch7", "Ch5", "Ch10"]`` returns identical data + under a reordered axis: same key, same channel count, different labels -- + the one thing a hash keyed on ``(key, n_ch)`` cannot see. + """ + n_times = 4 + data = np.tile(np.arange(len(labels), dtype=float), (n_times, 1)) + return AxisArray( + data, + dims=["time", "ch"], + axes={ + "time": AxisArray.TimeAxis(fs=100.0, offset=0.0), + "ch": AxisArray.CoordinateAxis(data=np.array(labels), dims=["ch"]), + }, + key=key, + ) + + +def test_slicer_relabelled_axis_reresolves_selection(): + """A label selection must follow its label when the axis is reordered. + + The channel count and the message key are unchanged, so state keyed on + those alone would keep the indices resolved against the previous axis -- + and, because the output axis is cached with them, keep labelling the wrong + channel's samples "Ch7" without raising anything. + """ + xformer = SlicerTransformer(SlicerSettings(selection="Ch7", axis="ch")) + + out = xformer(_labelled_msg(["Ch5", "Ch7", "Ch10"])) + assert out.data[0, 0] == 1 # Ch7 is at position 1 + assert np.array_equal(out.axes["ch"].data, np.array(["Ch7"])) + + out = xformer(_labelled_msg(["Ch7", "Ch5", "Ch10"])) + assert out.data[0, 0] == 0 # ... and now at position 0 + assert np.array_equal(out.axes["ch"].data, np.array(["Ch7"])) + + +def test_slicer_label_gone_from_axis_is_no_longer_selected(): + """Test relabeling. + A relabel that removes the selected channel must stop returning one. + """ + xformer = SlicerTransformer(SlicerSettings(selection="Ch7", axis="ch", on_empty="warn")) + xformer(_labelled_msg(["Ch5", "Ch7", "Ch10"])) + + out = xformer(_labelled_msg(["Ch1", "Ch2", "Ch3"])) + assert out.data.shape[1] == 0 + assert out.axes["ch"].data.size == 0 + + +def test_slicer_regex_selection_follows_a_relabel(): + xformer = SlicerTransformer(SlicerSettings(selection="C.*", axis="ch")) + assert xformer(_labelled_msg(["Fp1", "C3", "C4"])).data[0].tolist() == [1.0, 2.0] + assert xformer(_labelled_msg(["C3", "C4", "Fp1"])).data[0].tolist() == [0.0, 1.0] + + +def test_slicer_field_selection_follows_a_relabel(): + """Structured axes too: the field values are what the tokens matched.""" + dt = np.dtype([("bank", "U2"), ("elec", " AxisArray: + data = np.zeros(len(banks), dtype=dt) + for i, bank in enumerate(banks): + data[i] = (bank, i + 1) + return AxisArray( + np.tile(np.arange(len(banks), dtype=float), (4, 1)), + dims=["time", "ch"], + axes={ + "time": AxisArray.TimeAxis(fs=100.0, offset=0.0), + "ch": AxisArray.CoordinateAxis(data=data, dims=["ch"]), + }, + key="test_field_relabel", + ) + + xformer = SlicerTransformer(SlicerSettings(selection="B", axis="ch", field="bank")) + assert xformer(msg(["A", "B", "B"])).data[0].tolist() == [1.0, 2.0] + assert xformer(msg(["B", "A", "A"])).data[0].tolist() == [0.0] + + +def test_slicer_positional_selection_ignores_the_axis_values(): + """A slice selection cannot depend on the labels, so it must not re-resolve. + + Also the reason the hash may skip the axis entirely for such selections: + hashing coordinate data no positional selection can consult is pure cost. + """ + xformer = SlicerTransformer(SlicerSettings(selection="0:2", axis="ch")) + assert xformer(_labelled_msg(["Ch5", "Ch7", "Ch10"])).data[0].tolist() == [0.0, 1.0] + assert xformer(_labelled_msg(["Ch9", "Ch8", "Ch1"])).data[0].tolist() == [0.0, 1.0] + + +def test_slicer_unchanged_axis_does_not_reset_state(): + """An equal axis rebuilt per message must not look like a change. + + Sources commonly rebuild an identical ch axis for every message; if that + counted as a change, the selection would be re-parsed at the sample rate. + """ + xformer = SlicerTransformer(SlicerSettings(selection="Ch7", axis="ch")) + xformer(_labelled_msg(["Ch5", "Ch7", "Ch10"])) + first = xformer._hash + xformer(_labelled_msg(["Ch5", "Ch7", "Ch10"])) + assert xformer._hash == first