Resolve the operating axis from chunk_dim, and deprecate the axis settings - #239
Merged
Conversation
Processors guessed positionally for the dimension their settings did not name: dims[0] for "the streaming axis", dims[-1] for "the channel axis". Both are positions rather than meanings, and AxisArray.chunk_dim is the producer's declaration of which dimension messages accumulate along. The guess and the declaration disagreed under transpose and downstream of any windowing stage, so a processor's state-reset logic and its arithmetic could each pick a different axis: baseproc's _message_hash already excluded chunk_dim="win" while _reset_state built filter state along dims[0]. Three rules in util/message.py, because one does not fit every case: * resolve_chunk_dim -- the accumulating dimension, for stages that carry state between messages (filter zi, a running mean, a sample buffer, a previous-sample cache). Carrying that along a static axis is not a smaller error but a different operation. * resolve_feature_dim -- position -1 (or 0) skipping the chunk dim, for stages whose axis is static. dims[-1] can silently *be* the chunk dim on a (ch, time) stream, making AffineTransform matmul across time and Slicer discard samples. * resolve_transform_dim -- the innermost non-chunk LinearAxis, else the chunk dim. Spectrum needs "time" both on a raw (time, ch) stream and on windowed (win, time, ch), where win accumulates but each window's spectrum is over time. Downsample's private _resolve_axis collapses into the shared helper. Fixes along the way: * Spectrum picked dims[0] on windowed input, FFT-ing across windows rather than within each one. * diff.py's documented axis=None default raised "dim=None not present in object": __call__, __acall__ and _reset_state passed the raw setting to get_axis_idx, masked only because the diff() factory hardcoded "time". * The River scaler hardcoded axis_idx = 0 whenever axis was unset, which silently transposed a (ch, time) stream. * Merge passed align_axis or "time", so every Merge hardcoded the guess. Deprecate the axis settings (3.8, removed in 4.0) Where anything other than the chunk dimension cannot be meaningful, the setting can only be used to be wrong, so it is going away. 27 Settings classes get a __post_init__ hook; FilterBaseSettings covers ten filters and EWMASettings covers the scaler. ez.Settings are frozen dataclasses and Transformer(*args, **kwargs) funnels through _unify_settings, so one hook catches the settings class, the transformer, the unit and the factory. * FutureWarning, not DeprecationWarning: the latter is suppressed by default outside __main__, so pipelines -- library code -- would never see it. ezmsg core already uses FutureWarning for its own deprecations. * The stacklevel is computed by walking to the first frame whose module is outside ezmsg. A fixed level cannot work (the depth differs per construction path) and a filename test cannot either: the dataclass __init__ is exec-generated, so co_filename is "<string>", the same as `python -c`. Its f_globals name the defining module. * Behaviour is unchanged until removal; the setting is still honoured. * suppress_axis_deprecation() covers the stages that forward a setting internally. Without it every filter-by-design would warn on every state reset -- mid-stream, naming whatever drives the pipeline -- because FilterByDesignTransformer rebuilds a child FilterSettings each time. Merge and FBCCA forward their own align_axis/time_dim, settings the user cannot drop. Run a pipeline's tests with -W error::FutureWarning to locate call sites. Follow the same rule for the stages that hardcoded axis = "time" adaptive_lattice_notch, adaptive_lnc, binned_aggregate, resample, wavelets, rollingscaler and filterbankdesign all carry state along their axis but defaulted to a literal "time" rather than to a positional guess. Flipping them to follow chunk_dim changes results wherever the chunk dimension is not "time" -- downstream of a windowing stage -- and no setting exists to warn about, because the affected caller set nothing. resolve_configured_ chunk_dim grows a legacy_default parameter that surfaces exactly that population once, with the escape hatch of pinning axis="time" for now. Each of these used settings.axis at up to fifteen other sites, all of which break on a None default, so each now caches its resolved axis in state at reset. resample seeds it from push_reference, which is an independent entry point that can precede any signal message, and overwrites from the authoritative signal path; filterbankdesign resolves in _hash_message, which runs before _reset_state; binned_aggregate and resample had unit-level publish gates naming a different dim than the one that was processed. FilterbankDesign after a Window now raises rather than designing along a 6.25 Hz win axis -- an improvement, since a temporal filter along win is either wrong or very inefficient. The legacy-default warning fires first. FlattenSettings.preserve_axis stays configurable: Flatten holds no data between messages and already sets chunk_dim=None when the chunk dimension is folded into the merged axis, so preserving a non-chunk axis is coherent. Tests pin the three resolution rules, the deprecation contract, and the exact inventory of deprecated classes, so the 4.0 removal has a checklist.
`isinstance(tuple[int, str], type)` is True on 3.10 and False from 3.11, so a module-level generic alias -- align.py's `_AlignPair` -- got past inspect.isclass and raised "issubclass() arg 1 must be a class". Only the 3.10 matrix job saw it; fail-fast cancelled the other twelve, which made it look like a broad failure rather than one interpreter difference. Verified against a real 3.10.15 environment, not just by inspection.
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.
Follow-on to #235, which made
Downsample's dimension non-configurable. This applies the same reasoning everywhere else.The problem
Processors guessed positionally for the dimension their settings did not name —
dims[0]for "the streaming axis",dims[-1]for "the channel axis". Both are positions rather than meanings, andAxisArray.chunk_dimis the producer's declaration of which dimension messages accumulate along.The guess and the declaration disagree under
transposeand downstream of any windowing stage. That meant a processor's state-reset logic and its arithmetic could pick different axes:baseproc's_message_hashalready excludedchunk_dim="win"while_reset_statebuilt filter state alongdims[0].Three resolution rules
One rule doesn't fit every case, so
util/message.pygrows three:resolve_chunk_dimzi, running mean, sample buffer, previous-sample cache)chunk_dim→STREAMING_DIMS→dims[0]resolve_feature_dimresolve_transform_dimSpectrumLinearAxis, else the chunk dimDownsample's private_resolve_axiscollapses into the shared helper.The third exists because
Spectrumneedstimeon both a raw(time, ch)stream and a windowed(win, time, ch)one —winaccumulates, but each window's spectrum is taken overtime.Bugs fixed along the way
Spectrumpickeddims[0]on windowed input — FFT-ing across windows instead of within each one.diff.py's documentedaxis=Nonedefault raiseddim=None not present in object.__call__,__acall__and_reset_statepassed the raw setting toget_axis_idx; masked only because thediff()factory hardcoded"time".axis_idx = 0wheneveraxiswas unset, silently transposing a(ch, time)stream.Mergepassedalign_axis or "time", so everyMergehardcoded the guess.Deprecating the
axissettings (3.8 → removed in 4.0)Where anything other than the chunk dimension cannot be meaningful, the setting can only be used to be wrong. 27 Settings classes get a
__post_init__hook —FilterBaseSettingscovers ten filters,EWMASettingscovers the scaler.ez.Settingsare frozen dataclasses andTransformer(*args, **kwargs)funnels through_unify_settings, so one hook catches all four construction paths: settings class, transformer, unit, and functional factory.Three details worth reviewing:
FutureWarning, notDeprecationWarning. The latter is suppressed by default outside__main__, so pipelines — library code — would never see it. ezmsg core already usesFutureWarningfor its own deprecations.ezmsg. A fixed level can't work (depth differs per construction path), and a filename test can't either: the dataclass__init__is exec-generated, soco_filenameis"<string>"— indistinguishable frompython -c. Itsf_globalsname the defining module.test_a_configured_axis_is_still_honouredpins it.suppress_axis_deprecation()covers stages that forward a setting internally. Without it, every filter-by-design would warn on every state reset — mid-stream, naming whatever drives the pipeline — becauseFilterByDesignTransformerrebuilds a childFilterSettingseach time.MergeandFBCCAforward their ownalign_axis/time_dim, which users can't drop.To migrate a pipeline: run its tests with
-W error::FutureWarning; every remaining call site becomes a hard failure with a traceback.The
axis = "time"stagesadaptive_lattice_notch,adaptive_lnc,binned_aggregate,resample,wavelets,rollingscalerandfilterbankdesignall carry state along their axis but defaulted to a literal"time"rather than a positional guess. Flipping them changes results wherever the chunk dim isn't"time"— and no setting exists to warn about, because the affected caller set nothing.So
resolve_configured_chunk_dimgrows alegacy_defaultparameter that surfaces exactly that population, once, with the escape hatch of pinningaxis="time"during the window. Silent on raw streams and on streams with notimedim at all.Each of these used
settings.axisat up to fifteen other sites, all of which break on aNonedefault, so each caches its resolved axis in state at reset. Three needed more:resample—push_referenceis an independent entry point that can precede any signal message, and__next__has no message at all. It seeds from the reference path and overwrites from the authoritative signal path.filterbankdesign—_hash_messageruns before_reset_state, so it resolves from the message directly.binned_aggregate,resample— unit-level publish gates named a different dim than the one processed.Behaviour changes to be aware of
FilterbankDesignafter aWindownow raises rather than designing along a 6.25 Hzwinaxis. A temporal filter alongwinis either wrong or very inefficient, so failing loudly is the intent. The legacy-default warning fires first.wininstead oftimedownstream of a windowing stage — warned vialegacy_default.align/filterbankaxisdefaults changed"time"→None(this is what made theiror dims[0]dead code).Deliberately excluded
FlattenSettings.preserve_axisstays configurable. Flatten holds no data between messages and already setschunk_dim=Nonewhen the chunk dimension is folded into the merged axis, so preserving a non-chunk axis is coherent — closer toSlicerthan to a filter. Its default now resolves viaresolve_chunk_dim.Testing
4285 unit + 51 integration tests pass; ruff clean. Two new test modules pin the three resolution rules, the deprecation contract, and the exact inventory of deprecated classes — so adding or dropping one is a deliberate edit, and the 4.0 removal has a checklist.
Two things reviewers should know:
axis=at 249 distinct call sites. Deliberate: during the window they're the regression tests for the "still honoured" guarantee, and the new modules cover the default path. They'll need migrating at 4.0.riveris a lazy optional import and isn't installed, sotest_scaler_looks_up_the_axis_index_rather_than_assuming_zerois the one skip.