Slicer: normalize comma-separated selections — dedup always, axis order by default - #191
Merged
Merged
Conversation
A comma-separated selection resolved token by token and concatenated in token order, keeping duplicates. Selections naming the same channels in a different token order silently produced differently-ordered output of identical shape, and overlapping tokens duplicated channels so the coordinate axis carried repeated labels. SlicerTransformer now normalizes the resolved indices per a new `order` setting: - "axis" (default): indices are deduplicated and sorted into axis order, making a selection a filter — token order never matters and the coordinate axis stays unique. - "selection": entries keep token order for intentional permutations (e.g. "3,1,2"); duplicates are still removed, first occurrence wins. Normalization logs when it changes anything: a warning when overlapping tokens produced duplicates, an info message when token order differed from axis order under order="axis". A single non-comma slice token (e.g. "::-1") is applied as-is and not normalized. parse_slice is unchanged; normalization happens in _reset_state where the axis length is known. Fixes #188
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #188.
Problem
Slicerresolved a comma-separated selection token by token and concatenated the per-token indices in token order, keeping duplicates:".*-m1-.*,.*-aip-.*"and".*-aip-.*,.*-m1-.*"selected the same channels in different orders — same shape, no signal, so a downstream consumer (e.g. a trained decoder) reads every column as the wrong channel."3:10,1,7"duplicated index 7, so the outputchaxis carried a repeated label, breaking coordinate-axis uniqueness for any later label-based selection or channel-keyed stage.Solution
Per the issue's directions 3 + 4: a new
ordersetting onSlicerSettings(also exposed via theslicer()convenience function), with the safer semantics as the default, plus logging whenever normalization changes anything.order="axis"(default): after token resolution, indices are deduplicated and sorted into axis order. A selection is a filter — the same channel set always produces the same output regardless of token order, matching how label/regex selections read.order="selection": entries keep token order, so a positional"3,1,2"remains an intentional permutation. Duplicates are still removed (first occurrence wins) — a repeated channel on a coordinate axis is never what anyone meant.order="axis"(once per stream configuration, like theon_emptylogs).Since one selection string can mix positional and label tokens, one rule wins: the default treats the whole selection as a filter, and
order="selection"is the explicit opt-in for permutation semantics.Implementation notes
SlicerTransformer._reset_state(via a_normalize_indiceshelper), which is the one place that knows the resolved index array and axis length.parse_sliceis unchanged — its documented concatenate-in-token-order behavior (from Select channels based on their labels #61) remains, with a docstring note pointing at the transformer-level normalization."::-1","5:") still takes the plain-slice path and is not normalized, so explicit reverse/strided slices keep working; the issue is scoped to comma-separated selections.ordervalues raise at first message, mirroringon_emptyvalidation.Tests
"3:10,1,7"→ch01, ch03..ch09(dedup + axis order, both log messages asserted), and the m1/aip regex pair producing identical output in either token order.order="selection": permutation preserved, duplicates still dropped keeping first occurrence."::-1"untouched; invalidorderraises.