Axis identity: CoordinateAxis.fingerprint and AxisArray.chunk_dim - #265
Open
cboulay wants to merge 4 commits into
Open
Axis identity: CoordinateAxis.fingerprint and AxisArray.chunk_dim#265cboulay wants to merge 4 commits into
cboulay wants to merge 4 commits into
Conversation
An operation that resolves coordinate *values* into cached state -- channel labels into array indices, or into output labels -- cannot key that cache on shape alone. A source that renames, reorders or swaps channels without changing how many it sends keeps getting the previously resolved answer, and the operation silently emits one channel's samples under another channel's label. Comparing the arrays outright is O(bytes) on every message and in every consumer. `fingerprint` is a small hashable digest, computed on first access and cached on the instance, so the cost is paid once per axis object rather than once per consumer per message. Being derived rather than assigned, there is no counter for a producer to forget to bump; building a new axis is what changes it. The cached value lives in __dict__, so it is pickled with the axis and arrives already computed on the far side of a process boundary. Measured on a 256-channel ChannelMap axis, 150 messages through one process hop with five contents-dependent consumers: 750 digests computed without it, 1 with. crc32 rather than hash(tobytes()) because the copy is not the bottleneck -- on that axis the copy runs at ~94 GB/s while CPython's siphash over the result manages ~5.5 GB/s, and crc32 reads the array's buffer directly at ~29 GB/s. The dtype is stored as the object rather than str(dtype), which numpy builds field by field at roughly ten times the cost of the checksum it annotates. None is returned, and cached, when the contents cannot be digested; callers must treat that as "unknown" rather than as a value that compares equal to another None. fast_replace has to drop the cache. It copies __dict__ straight into the constructor and is called on axes, not only on messages, so a cached _fingerprint reaches CoordinateAxis.__init__ as an unexpected keyword and raises TypeError. Dropping is also the correct semantics: a digest of the old field values must not ride onto a copy that changes them.
CoordinateAxis inherits from two dataclasses that each supply an __eq__, and
the MRO picks the wrong one:
CoordinateAxis -> AxisBase -> ABC -> ArrayWithNamedDims
AxisBase is a plain @DataClass, so it generates an __eq__ over its only field,
`unit`, which shadows the content comparison in ArrayWithNamedDims. Two
coordinate axes therefore compared equal whatever their coordinate values
were:
axis(["A", "B", "C"]) == axis(["X", "Y", "Z"]) # True
and because AxisArray.__eq__ tests `self.axes == other.axes`, so did two
messages differing only in their channel labels.
Defines __eq__ explicitly on CoordinateAxis, comparing unit, dims and values,
and following ArrayWithNamedDims' convention of returning True or
NotImplemented so that AxisArray.__eq__'s dict comparison still resolves
correctly. LinearAxis equality is unaffected and the class remains unhashable,
as before.
Successive messages in a stream concatenate along one dimension, and that one is different in kind from the others: its length is however much arrived this time, a LinearAxis there has an offset that advances every message, and a CoordinateAxis there (irregular event times) has per-message values. Everything else describes the stream's configuration and is stable between reconfigurations. Consumers that cache state keyed on the message layout need that distinction and cannot reliably infer it. It is "time" on a raw signal but "win" downstream of a windowing stage, so a consumer that assumes "time" both thrashes on the window count and stops noticing a change in the window length. Taking dims[0] instead trades that for breaking under transpose, where it would exclude the channel dimension -- the very thing worth watching. Only the producer, which named the dims, reliably knows. Declaring it on the message puts the answer in that one place instead of asking each consumer to guess about a message it did not create. The field defaults to None, meaning "not declared", so existing producers are unaffected and consumers can tell silence from a real declaration. It is placed last, so positional construction is unchanged, and fast_replace carries it across a `replace(msg, data=...)` that keeps the same layout. Defaulting it to "time" instead was tried and rejected: 13 tests here and 91 in ezmsg-sigproc construct messages with no time dimension at all -- (win, freq, ch), (freq, ch), (bin, ch), (epoch, target_freq) -- so every one would have to opt out explicitly. Worse, a post-Window (win, time, ch) message *does* contain "time", so the default would pass validation while naming the wrong dimension, reintroducing the exact failure this field exists to prevent in the one case validation cannot catch. Validation lives in ArrayWithNamedDims.__post_init__ rather than an AxisArray override: reusing the existing frame costs one getattr (+0.036 us on message construction) where a second __post_init__ call costs +0.052, and construction is hot. It makes a rename that forgets to update chunk_dim raise at the site that has to handle it.
This was referenced Sep 3, 2026
`chunk_dim` names a dimension, so a transformer that renames or drops
dimensions has to move it along with the dimension it points at.
Left alone it is either stale-but-fatal -- `replace()` raises because the
old name is no longer in `dims` -- or, on a swap like
`{"win": "time", "time": "sample"}`, silently correct-looking while naming
a different dimension than before. The first form showed up as 26 failures
in ezmsg-sigproc's spectrogram and single-band-power composites once
`Window` started declaring `chunk_dim="win"` on its output.
Dropping a dimension clears `chunk_dim` rather than leaving it dangling.
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.
Stacked on #258 — please review/merge that first; the base here is
cboulay/surface_auto_start.Adds a content-derived identity to
CoordinateAxis, and fixes an equality bug found while building it.Motivation
An operation that resolves coordinate values into cached state — channel labels into array indices, or into output labels — cannot key that cache on shape alone. A source that renames, reorders or swaps channels without changing how many it sends keeps getting the previously resolved answer.
The failure is silent and takes two forms. A
SlicerorFlattenemits one channel's samples under another channel's label. A filter is worse, because its per-channel state is numeric: reconfiguring a device at a fixed channel count leaves the new channel's first samples dominated by the old channel's filter history —Consumers can already detect this by comparing the arrays, but that is O(bytes) on every message and in every consumer.
CoordinateAxis.fingerprintA small hashable digest, computed on first access and cached on the instance, so the cost is paid once per axis object rather than once per consumer per message.
It is derived, not assigned. There is no counter for a producer to forget to bump — building a new axis is what changes it. That was the deciding factor over a
generationint: a derived value cannot be forgotten, whereas a missed bump would be a silent stale-cache bug carrying a false guarantee.Because the cached value lives in
__dict__, it is pickled with the axis and arrives already computed on the far side of a process boundary. Verified through ezmsg's real transport (separate process, shared memory +Marshal), inspecting__dict__before anything touches the property: all 20 messages arrived precomputed. Measured on a 256-channel ChannelMap, 150 messages through one hop with five contents-dependent consumers: 750 digests without it, 1 with.Implementation notes, all measured on that axis:
hash(tobytes())— the copy is not the bottleneck. The copy runs at ~94 GB/s while CPython's siphash over the result manages ~5.5 GB/s; crc32 reads the array's buffer directly at ~29 GB/s. Net 5.0 µs → 0.95 µs.str(dtype)— numpy builds a structured dtype's repr field by field, which cost 10 µs for an 8-field ChannelMap, ten times the checksum it was annotating. The object is hashable and value-comparing at 0.03 µs.Nonemeans "unknown", and is itself cached. Callers must fall back to comparing rather than treating twoNones as equal.Cost: 1.06 µs first access, 0.051 µs cached.
fast_replacedrops the cachefast_replacecopies__dict__straight into the constructor, and it is called on axes, not only on messages —replace(message.axes[axis], data=...)appears in four ezmsg-sigproc modules. A cached_fingerprinttherefore reachesCoordinateAxis.__init__as an unexpected keyword and raisesTypeError.Dropping is also the correct semantics: a digest of the old field values must not ride onto a copy that changes them.
slow_replacewas already fine, since it only passes init fields.Two approaches were measured; the targeted pop is 13× cheaper than filtering:
replace()_-prefixed keysBug fix:
CoordinateAxis.__eq__compared onlyunitFound while writing the tests above.
CoordinateAxisinherits from two dataclasses that each supply an__eq__, and the MRO picks the wrong one:AxisBaseis a plain@dataclass, so it generates an__eq__over its only field,unit, shadowing the content comparison inArrayWithNamedDims. So:and since
AxisArray.__eq__testsself.axes == other.axes, two messages differing only in their channel labels compared equal too.Fixed by defining
__eq__explicitly, followingArrayWithNamedDims' True-or-NotImplementedconvention soAxisArray.__eq__'s dict comparison still resolves correctly.LinearAxisequality is unaffected and the class remains unhashable.This is a behaviour change for anyone relying on the loose comparison. It immediately surfaced a latent gap in ezmsg-sigproc's buffer-recycling test helper, which had been unable to detect a retained coordinate axis because the comparison it relied on was the shadowed
__eq__.Testing
24 new tests. Both commits are independently green.
tests/messages/186 passed. Full suite 454 passed, with one pre-existing failure (test_perf_analysisneedsxarray, fails identically at the base commit). Lint and format at exact parity on the touched files.Notes for reviewers
__setattr__guard invalidating the cache on field assignment. It costs 3.7× on construction and would catchax.data = otherbut notax.data[0] = "X", which is the same contract violation. Documented instead — the axis must not be mutated after construction, which fan-out to multiple graph branches already required.LinearAxisdeliberately gets no fingerprint: it already compares by value cheaply, and giving it one invites folding a per-messageoffsetinto a state hash.🤖 Generated with Claude Code
Update: third commit adds
AxisArray.chunk_dimReviewing the above surfaced a gap the fingerprint alone doesn't close. A consumer that caches state keyed on the message layout has to know which dimension is the one messages append along — its length is whatever arrived this time, a
LinearAxisthere has an offset that advances every message, and aCoordinateAxisthere (irregular event times) has per-message values. Everything else describes the stream's configuration.Consumers can't reliably infer it. It's
"time"on a raw signal but"win"downstream of a windowing stage, so assuming"time"both thrashes on the window count and stops noticing a change in the window length:Taking
dims[0]instead trades that for breaking undertranspose, where it would exclude the channel dimension — the very thing worth watching. Only the producer, which named the dims, reliably knows.chunk_dimdefaults toNone("not declared"), so existing producers are unaffected and consumers can tell silence from a real declaration. It's placed last, so positional construction is unchanged, andfast_replacecarries it across areplace(msg, data=...)that keeps the same layout.Defaulting it to
"time"was tried and rejected. 13 tests here and 91 in ezmsg-sigproc construct messages with no time dimension at all —(win, freq, ch),(freq, ch),(bin, ch),(epoch, target_freq)— so every one would have to opt out explicitly. Worse, a post-Window(win, time, ch)message does contain"time", so the default would pass validation while naming the wrong dimension, reintroducing the exact failure the field exists to prevent, in the one case validation cannot catch.Validation lives in
ArrayWithNamedDims.__post_init__rather than anAxisArrayoverride — reusing the existing frame costs onegetattr(+0.036 µs on message construction) where a second__post_init__call costs +0.052, and construction is hot. It makes a rename that forgets to updatechunk_dimfail at the site that has to handle it:Testing (all three commits)
Each commit is independently green.
tests/messages/193 passed; full suite 461 passed with one pre-existing failure (test_perf_analysisneedsxarray, fails identically at the base commit). Lint and format at exact parity on the touched files.Downstream
ezmsg-baseproc consumes both fields: its default
_hash_messagefolds in the coordinate fingerprints and excludes the chunk dimension, falling back to a class-level("time",)when a message doesn't declare one. Verified against ezmsg-sigproc's full suite (4171 passed) with no regressions, and still passing against published ezmsg, so the two can be released independently.Update: fourth commit carries
chunk_dimthroughModifyAxisTransformerchunk_dimnames a dimension, so anything that renames or drops dimensions has to move it along with the dimension it points at.ModifyAxisTransformerdid not, which left two failure modes:dims, so thereplace()at the end of__call__trips the validation added in the previous commit. This is how it surfaced: 26 ezmsg-sigproc failures in the spectrogram and single-band-power composites, which renamewinback totimeafter windowing.{"win": "time", "time": "sample"}— the old name is still indims, so validation passes whilechunk_dimnow names a different dimension than it did before. Exactly the mislabelling the field exists to prevent.Dropping a dimension clears
chunk_dimrather than leaving it dangling.Five new tests covering rename, swap, an unrelated rename, undeclared, and drop.
tests/messages/198 passed; full suite 466 passed with the same one pre-existingtest_perf_analysisfailure. With this commit, ezmsg-sigproc's suite is green (4184 passed) against this branch.