Report scaling as flat attrs so generic attrs stages can read it - #27
Merged
Conversation
A message reported its scaling as one nested dict under attrs["nwb_scaling"]. That is opaque to any stage that walks attrs generically, and ezmsg-sigproc's concat rejects it outright: _check_attr_type allows only scalars, so a dict raised TypeError even when both messages carried an identical one, and even when only one side had the key. Any concat touching an NWB-sourced stream failed. Report five prefixed scalars instead -- nwb_scaling_gain, _offset, _unit, _applied, _voltage. The prefix keeps the namespacing that argued against flat attrs["conversion"]/attrs["offset"] in the first place. Flat keys also let concat do the right thing rather than merely not crash: when two streams' gains differ it promotes nwb_scaling_gain onto the ch axis, which is where a per-channel gain belongs. A dict could never express that. StreamScaling.as_attr/from_attr become as_attrs/from_attrs, taking and returning whole attrs; from_attrs returns None when the message reports no scaling, replacing the get()-then-None-check at every call site. Known hole, left deliberately: a genuinely per-channel gain is an ndarray, which _ALLOWED_ATTR_SCALARS also rejects, so it still cannot ride in attrs through concat. Only reachable when a file's channel_conversion entries differ -- uniform ones collapse to a scalar in resolve_scaling, and every stream in the session files on hand does. Covered by an xfail-style test and filed upstream. The per-message fast path is not a casualty. It cached on the identity of the single dict, which five keys removed; _hash_message now keys on the identity of the attrs dict itself and _reset_state re-validates by value, with _Plan holding the dict so no other object can take its address. Measured on 30x64 int16 messages: 47.3 ns/msg against 57.2 ns before, and a plan is no longer rebuilt when an upstream stage rebuilds attrs without changing the scaling. Wire-format break with no external consumers yet.
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.
A message reported its scaling as one nested dict under
attrs["nwb_scaling"]. That is opaque to any stage that walks attrs generically, andezmsg-sigproc'sconcatrejects it outright —_check_attr_typeallows only scalars, so a dict raisedTypeErroreven when both messages carried an identical one, and even when only one side had the key. Any concat touching an NWB-sourced stream failed.Change
Report five prefixed scalars instead:
nwb_scaling_gain,_offset,_unit,_applied,_voltage. The prefix keeps the namespacing that argued against flatattrs["conversion"]/attrs["offset"]in the first place.Flat keys also let concat do the right thing rather than merely not crash: when two streams' gains differ it promotes
nwb_scaling_gainonto thechaxis, which is where a per-channel gain belongs. A dict could never express that.StreamScaling.as_attr/from_attrbecomeas_attrs/from_attrs, taking and returning whole attrs.from_attrsreturnsNonewhen the message reports no scaling, replacing theget()-then-None-check at every call site.The fast path is not a casualty
convert.pycached on the identity of the single dict, which five keys removes._hash_messagenow keys on the identity of the attrs dict itself,_reset_statere-validates by value, and_Planholds the dict so no other object can take its address — the same safety argument the original code used.Measured on 30x64 int16 messages at simulated-online sizes:
id)It also fixes a latent cache miss: a plan is no longer rebuilt when an upstream stage rebuilds attrs without changing the scaling.
Known hole, left deliberately
A genuinely per-channel gain is an
ndarray, which_ALLOWED_ATTR_SCALARSalso rejects, so it still cannot ride through concat. Only reachable when a file'schannel_conversionentries differ — uniform ones collapse to a scalar inresolve_scaling, and every stream in the session files on hand does. Pinned by a test that asserts the current failure, and filed upstream against ezmsg-sigproc.Tests
test_sigproc_interop.py(new) — the concat regression, gain promotion onto thechaxis, and the vector-gain hole. Skipped unless thesigprocextra is installed; verified passing against ezmsg-sigproc on the path.test_scaling.py— the flat shape is now asserted as a contract (no value is a container), plusas_attrs/from_attrsround-trip and the no-scaling case.test_convert.py— rebuilt-attrs reuses the plan; a genuinely changed scaling still rebuilds it.276 passed, 1 skipped, ruff clean. Verified on
sub-CA001/ses-S006: all five keys present as scalars and round-tripping.Wire-format break, with no external consumers yet.