Repository navigation
Follow chunk_dim for binning, and deprecate BinnedEventAggregatorSettings.axis - #38
Merged
Merged
Conversation
…ings.axis Binning carries an open partial bin across message boundaries, so it is only meaningful along the dimension messages accumulate along. That dimension is the producer's to declare -- `AxisArray.chunk_dim` -- and a setting letting a consumer disagree can only be used to be wrong. Deprecated here, removed in 2.0, matching what ezmsg-sigproc 3.8 does to the binner this delegates to. The default changes from "time" to None, so `BinnedAggregate` needs to be the version that resolves chunk_dim rather than indexing `axes[None]`; hence the ezmsg-sigproc>=3.8.0 pin. Two forwarding sites are wrapped in suppress_axis_deprecation(): * binned.py hands our own already-warned `axis` to BinnedAggregateSettings. Warning again would name a sigproc class for a setting the user made on ours, sending them to look in the wrong place. * peak.py builds an AdaptiveStandardScaler with axis="time". This module works on "time" throughout, and the scaler shares the state that normalises the samples being thresholded, so it must use the same axis. Suppressed rather than dropped for that reason -- letting the scaler follow chunk_dim while everything around it used "time" would be worse than either. Teaching the whole module to resolve chunk_dim is a separate change.
ezmsg-sigproc 3.8.0 shipped its own copy of the deprecation machinery before it moved to ezmsg-baseproc, so its warnings check a ContextVar that baseproc's suppress_axis_deprecation() does not set. Entering baseproc's context therefore silenced nothing, and forwarding our axis warned twice -- once for our setting and once for sigproc's. Importing the context manager from ezmsg.sigproc works against 3.8.0 and against later versions, which re-export baseproc's (so it is the same object and the same ContextVar). Our own settings keep warning through baseproc.
ezmsg-sigproc 3.8.1 shares baseproc's deprecation mechanism, so there is a single ContextVar again and the import that works is simply the obvious one. Pinned to >=3.8.1 accordingly, and the note explaining which of two mechanisms to reach for is gone with it.
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
1 participant
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.
Why
Binning carries an open partial bin across message boundaries, so it is only meaningful along the dimension messages accumulate along. That dimension belongs to the producer —
AxisArray.chunk_dim— and a setting that lets a consumer disagree can only be used to be wrong.BinnedEventAggregatorSettings.axisis deprecated here and removed in 2.0, matching what sigproc 3.8 does to theBinnedAggregatethis delegates to.The pin bump is load-bearing
The default changes from
"time"toNone. OldBinnedAggregateindexesmessage.axes[None]and raisesKeyError: None— I hit exactly that running the suite against released sigproc 2.31.0 (15 failures). So this needs the version that resolveschunk_dim, henceezmsg-sigproc>=3.8.0.Two forwarding sites suppressed
Without this, sigproc 3.8 would make existing, correct ezmsg-event pipelines emit
FutureWarnings naming sigproc classes the user never touched:binned.pyhands our own already-warnedaxistoBinnedAggregateSettings. Warning twice for one user-visible setting sends people to look in the wrong package.peak.pybuilds anAdaptiveStandardScalerwithaxis="time". This module works on"time"throughout (seven hardcoded uses), and the scaler shares the state that normalises the samples being thresholded — so it has to use the same axis. Suppressed rather than dropped for that reason: letting the scaler followchunk_dimwhile everything around it used"time"would be worse than either.Teaching
peak.pyto resolvechunk_dimthroughout is a real follow-up, but it's a behaviour change to threshold crossing and deserves its own PR.Testing
170 passed. New tests cover: setting
axiswarns about our class rather than sigproc's; leaving it unset is silent through processing (which is what the suppression buys); forwarding warns once, not twice; and a configured axis still bins identically, so behaviour is unchanged until removal.