BinnedAggregate: add a passthrough flag - #197
Merged
Merged
Conversation
There was no way to leave this node in a graph but switch it off. Every other conditioning stage has one -- CommonRereference has mode="passthrough", Butterworth has order=0, SamplingDelayAlignment has filter_len=0, Slicer has an empty selection -- so a pipeline that wants to compare with and without binning had to rewire itself instead. A separate flag rather than a sentinel bin_duration, because the intended use is a runtime toggle: a consumer switching this off because the view zoomed to a range where binning would cost detail should not have to remember and restore the rate itself. A sentinel would have destroyed it. passthrough is part of _hash_message, so flipping it resets the schedule and the carry. That matters: the carry holds an open partial bin, and if it survived the gap the first bin after resuming would mix samples from either side of it. There is a test that fails without the hash change. Note the shape consequence, which is documented on the setting: with a tuple operation, toggling adds and removes a trailing axis. Downstream has to absorb a rank change -- a fixed-layout sink reallocates, a plot rebuilds. ezmsg-tools' shmem bridge and sweep widget both do, but neither does it for free.
Putting passthrough in _hash_message worked, but it paid for the toggle on every message: each one hashed the flag, and the passthrough branches in _reset_state and _process existed only to describe a node that is supposed to be doing nothing at all. A message arriving in passthrough still walked the whole stateful path to be handed back unchanged. Short-circuit in __call__/__acall__ instead, matching EWMATransformer and AdaptiveStandardScalerTransformer, so passthrough returns the input before hashing and the state is never consulted. That removes the three branches. The reason passthrough was in the hash in the first place still holds: the carry holds an open partial bin, and if it survived the gap the first bin after resuming would mix samples from either side of it. Short-circuiting alone would have reintroduced exactly that, since _hash keeps its pre-gap value when nothing hashes it. So the passthrough branch calls _request_reset(), which invalidates _hash directly -- the next real message is guaranteed to take the reset path. _request_reset() goes in __call__ rather than only in update_settings so the guard also holds when settings are pushed by assignment (proc.settings = replace(...)) rather than through update_settings. That is what the existing toggle test does, and it is the path a runtime consumer is most likely to take. passthrough joins NONRESET_SETTINGS_FIELDS to say that update_settings need not queue a reset of its own. Note this is the opposite of what EWMA and the scaler do -- they resume from pre-gap state on purpose. The difference is that their state is a converged estimate, while the carry is raw samples that would be spliced into an output bin. Whether theirs is right is #195.
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.
What
Adds a
passthroughflag toBinnedAggregateSettings, so this node can be left in a graph but switched off at runtime.There was no way to do that before. Every other conditioning stage has one —
CommonRereferencehasmode="passthrough",Butterworthhasorder=0,SamplingDelayAlignmenthasfilter_len=0,Slicerhas an empty selection — so a pipeline that wanted to compare with and without binning had to rewire itself instead.A separate flag rather than a sentinel
bin_duration, because the intended use is a runtime toggle: a consumer switching this off because the view zoomed to a range where binning would cost detail should not have to remember and restore the rate itself. A sentinel would have destroyed it.How
__call__/__acall__short-circuit before the state is ever consulted, matchingEWMATransformerandAdaptiveStandardScalerTransformer:The
_request_reset()is the load-bearing part._state.carryholds the raw samples of an open partial bin; if it survived a passthrough gap, the first bin after resuming would mix samples from either side of it. Short-circuiting alone would cause exactly that, because_hashkeeps its pre-gap value when nothing hashes it._request_reset()sets_hash = -1directly, so the next real message is guaranteed to take the reset path and start from a fresh schedule and an empty carry.It lives in
__call__rather than only inupdate_settingsso the guard also holds when settings are pushed by assignment (proc.settings = replace(...)), which is the path a runtime consumer is most likely to take.passthroughis inNONRESET_SETTINGS_FIELDSto record thatupdate_settingsneed not queue a reset of its own.Note for downstream
Toggling changes the shape of the output when
operationis a tuple: the trailing metric axis appears and disappears with the flag. Anything downstream has to absorb a rank change — a fixed-layout sink reallocates, a plot rebuilds. ezmsg-tools' shmem bridge and sweep widget both do, but neither does it for free.Deliberately not doing
EWMATransformerandAdaptiveStandardScalerTransformerresume from pre-gap state after passthrough, which is the opposite of what this PR does. That is intentional there and pinned by tests. The difference is that their state is a converged estimate, while the carry is raw samples that get spliced into an output bin. Whether theirs is right is #195.Related: #196, an unrelated stale-cache bug found in
RangedAggregateTransformerwhile comparing passthrough implementations.Tests
Six tests in
tests/unit/test_binned_aggregate.py:test_passthrough_forwards_untouched— output is the input objecttest_passthrough_keeps_the_bin_rate_for_when_it_is_switched_back— the flag-vs-sentinel argumenttest_passthrough_skips_the_state_entirely— no schedule built, no carry growntest_toggling_off_does_not_splice_stale_samples_into_the_first_bin— the one that fails without_request_reset()test_toggling_off_via_update_settings_also_resets— the other toggle pathtest_toggling_restores_the_metric_axis— the trailing axis comes back correctlyFull suite passes (3689 unit + 51 integration).