Report what an NWB file's samples mean, and convert them in the graph - #22
Merged
Conversation
An NWB TimeSeries stores integer samples plus the factors that turn them into the unit it declares: value = data * conversion * channel_conversion + offset. This package read data and ignored the other three, handing every caller raw ADC counts labelled as volts. Nothing raises, because counts and volts are both just numbers -- but any downstream stage with an absolute threshold (a rail guard, a power clip) is then comparing against the wrong scale, and a chain fed from a live source and one fed from a recording of that same source silently disagree by the gain. NWBSlicer now resolves those factors per stream and applies them lazily on read, via a wrapper around the h5py dataset rather than a conversion at each call site -- the iterator's chunk builder and both slice methods all go through info.dset, so one wrapper covers them and none can forget. float32, not float64: broadband is ~80 GB per recording-hour and float32 already carries more mantissa than the int16 going in. The declared unit cannot be trusted, and neither can the conversion. All four of (0.25, microvolts), (1.0, volts, with channel_conversion 5e-08), (0.25, volts) and (1.0, uV) have been seen on files describing the same hardware at the same 0.25 uV/count -- the second being a writing library's default gain for some other amplifier, 5x off here and indistinguishable by any reader from a real one. So scale_override and unit_override let a caller who knows a writer lies say so, per stream or for the whole file, without patching the file. The unit is read off the h5py attribute rather than through pynwb on purpose: unit is a fixed field on ElectricalSeries in the NWB schema, so pynwb answers "volts" for every electrical series regardless of what is on disk. Believing it is a factor of 1e6. apply_conversion defaults to True. It is a breaking change for anyone relying on getting counts, and it is the right default anyway: the failure mode it removes is silently-wrong units, which is exactly the class of bug that does not announce itself. Pass False to reproduce a result computed before this. A stream whose scaling is the identity is left alone rather than copied into float32, so integer marker channels keep their dtype. test_writer_roundtrip_continuous now asserts the round trip is unit-correct (NWBSink declares conversion=1e-6, so a write-read cycle returns volts for the microvolts it was given) plus a second read with apply_conversion=False showing the stored samples are untouched.
The previous commit made reads honour what a file says its samples mean. That still leaves every file dictating the unit on the wire, which is backwards for a pipeline: a graph is written against one scale, and the files feeding it disagree -- some volts, some microvolts, all describing the same amplifier. Correcting that downstream means every consumer carrying a per-file factor, which is the same silently-wrong-units failure one layer up. target_unit converts an electrical stream to a requested VoltageUnit (volts, millivolts, microvolts, nanovolts) after the file's own factors resolve. The ratio folds into the gain, so it costs nothing on read -- the same single multiply, with a different constant. Voltage only, and only on ElectricalSeries. A prefix change is pure arithmetic on the samples; anything crossing a dimension needs a model of the circuit and belongs in a processing stage that can be told about it, not in a reader inferring one from a label. Whether a stream qualifies is asked of the object rather than of its declared unit -- being an ElectricalSeries is a structural fact about the file, while the unit string is exactly the thing this module does not trust. A marker stream declaring "n/a" is therefore left alone rather than erroring or, worse, being scaled by 1e6. Applied after scale_override and unit_override, not instead of them: the overrides establish what the file actually holds, target_unit converts from that to what the caller asked for. So a file that lies about both its gain and its label can be corrected once and then requested in any unit -- unit_override="volts" with target_unit="microvolts" is a net 1e6, where believing the file's "microvolts" would have been a no-op. The offset is scaled along with the gain. It is an additive constant in the same unit as the values, so converting only the gain would leave a stream with a non-zero offset wrong by a power of ten -- worse than not converting, because it looks converted. An unrecognized declared unit raises rather than passing the data through under the requested label; a caller who knows better has unit_override. Empty is the exception: the NWB schema fixes ElectricalSeries.unit to volts, so a writer that stamped nothing has said volts by omission. target_unit with apply_conversion=False raises too -- they ask for opposite things and raw counts have no unit to convert from. VoltageUnit subclasses str so settings survive a YAML round trip, and any spelling is accepted (uV, µV, microvolt) so nobody has to guess which one this module canonicalized on. MICROVOLT_UNITS now derives from that same table instead of duplicating it. NWBSlicer.close now reads _io via getattr: __del__ also runs on an object whose __init__ raised, and reading the attribute directly turned the real error into an AttributeError from a destructor, surfacing as an unrelated warning at an unrelated time.
…lSeries target_unit gated on isinstance(child, ElectricalSeries), reasoning that the declared unit is the thing this module does not trust. That conflates two claims the unit string makes. It is unreliable about magnitude -- that is what the table of contradictions is about, and what scale_override and unit_override exist for -- but about dimension it is all there is, and a wrong dimension is not a failure mode writers exhibit. Nobody labels a cursor position in volts. Gating on the type alone therefore missed voltage that is not an ElectricalSeries, which is not a corner case: a re-timestamped companion of an acquisition stream is written as a bare TimeSeries pointing at the very same HDF5 dataset. Asking for microvolts converted the acquisition stream and left its companion in volts -- the same bytes on disk delivered at two scales to the same graph, with nothing raised. That is the failure target_unit was added to remove, reintroduced by the gate meant to make it safe. A stream is convertible now if it is an ElectricalSeries or its declared unit parses as a voltage. The two pieces of evidence are independent and either suffices: the type is structural and covers a series that declares nothing (an ElectricalSeries is voltage by schema), the label is the only evidence a plain TimeSeries offers. Streams naming something else (pixels, n/a) still parse to nothing and are still left alone, so the narrowing that mattered is kept while the one that hurt is dropped. The benefit of the doubt does not extend to an unstamped plain TimeSeries: an ElectricalSeries with no unit means volts by schema, but an empty unit on a bare TimeSeries says nothing, and assuming volts there would invent a dimension rather than read one. The fixture grows a voltage TimeSeries and a pixels one, so the two halves of the gate are pinned by a test each. The pixels stream carries a non-identity conversion on purpose -- otherwise "left alone" would pass whether the unit was consulted or not.
apply_conversion=False handed back stored counts with an empty attrs: no
gain, no offset, no unit, no marker saying the values were unscaled. The
factors were not merely withheld, they were never read -- maybe_scale was
skipped entirely -- so the mode was a one-way door. A stage wanting to
scale later, after decimation or on a GPU, had to reopen the NWB file to
recover what the reader had already held.
Messages now carry attrs["nwb_scaling"] = {gain, offset, unit, applied}
in every mode. "Don't apply this" and "forget this" are different
requests and only the first was ever being asked.
The reported figures are the total transformation from stored samples to
delivered values, with scale_override and target_unit folded in, not the
file's raw attributes. A stage that trusted raw attributes would undo the
correction it was handed; what a consumer needs is the relationship
between the numbers it holds and the file, which is what this is.
attrs["unit"] stays absent when nothing was applied. It describes the
data, and the data is counts -- naming them volts is the bug this module
exists to prevent. The unit the values would reach lives inside
nwb_scaling, where it reads as part of a pending conversion rather than
as a claim about what the caller is holding.
gain is a scalar or a vector, mirroring resolve_scaling: uniform
per-channel factors collapse, so the common case reports one number
instead of an array a downstream stage would have to keep aligned with
the ch axis across every channel selection. When the channels genuinely
disagree it stays a vector, since summarizing it would be a lie. That
puts a numpy array inside attrs, a shape nothing else in these messages
has, so a test pins that it survives the message codec.
Namespaced under one key rather than flat conversion/offset/unit entries:
attrs is shared with every stage downstream and three unqualified generic
words are a collision waiting to happen.
maybe_scale becomes resolve_stream_scaling, taking apply as an argument
and returning (dset, unit, scaling). The resolve-always/apply-sometimes
split now lives in one function instead of being spread across a caller's
if-statement, which is what let the no-apply path silently skip
resolution in the first place. StreamInfo carries the same object.
apply_conversion=False now reports its factors, but nothing consumed
them. NWBScalingUnit does: it reads attrs["nwb_scaling"] and does exactly
what the reader would have, wherever it sits in the graph.
Deferring is worth doing because reading raw is the only way to keep
broadband int16 through the stages that do not care about units. Scaling
at the reader doubles every message to float32 immediately -- ~80 GB per
recording-hour becoming ~160 -- and a graph that decimates, subsets
channels, or windows before it needs volts pays that on samples it is
about to discard. Put this after those stages and only the survivors are
converted.
Settings mirror the reader's names and forms (conversion_dtype,
scale_override, unit_override, target_unit, each a bare value or a
{stream_key: value} mapping keyed on msg.key), so moving the work down
the graph is moving the settings rather than rewriting them. Tests pin
the equivalence directly: read raw, transform, and compare against
reading with the same settings applied at the source.
Stateless and idempotent, off the applied flag rather than the dtype. Two
in a chain, or one behind a reader that already converted, cannot
double-scale. Guessing from whether the data looks integral would be the
same silent-inference this whole line of work removes.
target_unit is honoured on an already-scaled message where the gain
overrides are not. A prefix change on top of a known unit is one
multiply: for value = stored * gain + offset, scaling the values carries
the offset with them, so nothing is unwound -- hence unit_ratio, split
out of convert_to_target_unit for the case where only the factor is
wanted. Correcting a gain already applied would instead mean dividing it
back out, a pass over the data and accumulated float error spent fixing
something the reader was better placed to fix, so scale_override and
unit_override raise there and say where to set them.
StreamScaling gains a voltage flag, decided by is_voltage_stream at read
time and carried on the message. One of that predicate's two inputs does
not survive the trip: an ElectricalSeries that stamped no unit is voltage
by schema, but off the file a consumer sees only an empty string.
Deciding where both the type and the label are in hand lets a later stage
convert exactly the streams the reader would have -- the cursor stream
stays pixels.
A per-channel gain is positional, so a stage that dropped or reordered
channels upstream has invalidated it. Length is the only check available
and it catches the common case; a misaligned multiply that happened to
broadcast would be silently wrong, so a mismatch raises and names the
cause.
The scaled_nwb_path fixture moves to conftest since two modules now read
it -- which is the point, as the equivalence being asserted only means
something against one file.
Measured on 256-channel int16 at 30 kHz, per message:
before after multiply-only floor
1 s (30000x256) 2702 us 2529 us 2427 us
100ms ( 3000x256) 237 us 244 us 230 us
30smp ( 30x256) 8.36 us 3.99 us 2.90 us
Offline was never the problem: at 1 s chunks the overhead was already
under 10% and is now noise against the multiply. The 30-sample case is
where it mattered -- 5.3 us of bookkeeping around 3.1 us of arithmetic --
and that is down to 1.1 us.
The suspect was not the culprit. StreamScaling.from_attr costs 0.33 us of
the 5.3; the single largest item was is_identity_scaling at 1.59 us,
which called np.all(np.asarray(gain) == 1.0) on a Python float -- two
array allocations and a reduction to answer a question == answers. It now
short-circuits on the scalar case, 1.59 -> 0.04 us, and that fix helps
every caller including the reader.
The rest was per-message re-derivation of things that cannot change
between messages: resolving overrides, parsing the target unit, rebuilding
the output attrs dict. NWBScalingTransformer becomes a
BaseStatefulTransformer that caches a resolved plan -- gain pre-cast to
the output dtype and pre-reshaped for broadcast, offset as None when zero
so the common case skips a pass, and the complete output attrs prebuilt.
_process is now a dict lookup, one multiply, and a replace.
The plan cache is per stream key rather than a single current plan,
because one reader publishes every stream on one output. With a single
plan two interleaved streams would evict each other on every message and
the cache would cost more than it saved; verified on a real recording that
two interleaved 256- and 128-channel streams come out bit-identical to
scaling at the reader.
Cache validation holds a reference to the attrs payload it was built from
and compares identity. Holding it is the point: an object we hold cannot
be freed and have its address reused by a different payload, which is the
trap in caching on a bare id(). _hash_message may therefore be as sloppy
as it likes -- it uses id() -- because a collision costs one dict lookup
in _reset_state, which re-validates, and not a wrong scaling.
The per-channel length check stays per message rather than moving into the
plan: the plan is keyed on the reported scaling, and an upstream channel
selection invalidates a positional gain without changing what the message
reports about it.
On the question of whether the iterator could stop scaling and defer to
this module: end to end on a real recording at 1 s chunks, reading raw and
transforming is 4.15 ms/chunk against 4.43 ms/chunk scaling at the reader,
over a 4.06 ms floor for reading int16 and not scaling at all. So routing
through convert.py is not a tax -- it is marginally cheaper, since the
iterator then moves int16 rather than float32 and only the surviving
message is widened. That removes the performance argument for the change,
which leaves only the correctness one, and that argument runs the other
way: apply_conversion=True is the default because silently-wrong units is
the failure being designed out. Keeping both, with the reader's default
intact, costs nothing measurable.
Two implementations of one conversion, five settings duplicated across three classes, and a reader that had to be told which of two things it was. Since the measurement showed deferring costs nothing -- 4.15 vs 4.43 ms/chunk at 1 s, because the graph then moves int16 until something actually needs volts -- the duplicate can go. NWBSlicer, NWBIteratorSettings and NWBClockDrivenSettings lose apply_conversion, conversion_dtype, scale_override, unit_override and target_unit. ScaledDataset and unwrap_dataset go with them, and with the wrapper gone so does its subtlety: _data_addr no longer has to unwrap before comparing HDF5 addresses, a step whose omission would have quietly demoted the dejitter pass's structural pairing check to the name convention it exists to back up. resolve_scaling drops its override and target arguments and reports the file verbatim. iterator.py and clockdriven.py are now byte-identical to dev. slicer.py's net addition drops from 125 lines to 51 -- a StreamInfo field, the describe call, and a docstring. The whole conversion lives in one module that does nothing else. This also removes the breaking change. Those five settings only ever existed on this branch, so deleting them leaves the reader returning exactly what dev returns: stored counts. The difference is now purely additive -- messages carry attrs["nwb_scaling"] -- and nobody's existing results move. What used to be a silent behaviour change becomes an opt-in one line in the graph. The cost is that a naive read still gets counts, which is the bug this line of work started from. It is a smaller cost than it looks: the failure mode before was counts labelled volts, and now nothing claims to be volts until something has made it so. attrs["unit"] appears only after the transformer runs, and until then the message says what it is and what it would take. Reader docstrings say so at the top. resolve_stream_scaling becomes describe_stream_scaling, which returns a StreamScaling or None and cannot apply anything. applied is always False coming out of the reader; it turns True only where a multiply happened. Tests split the same way. test_scaling.py checks what the reader reports and never that it converts; test_convert.py checks conversion against the NWB definition directly -- data * conversion * channel_conversion + offset -- rather than against a second implementation, since there is no longer one to agree with. The writer round trip asserts the file's samples come back unchanged and that running the transformer is what makes them volts.
cboulay
changed the title
…shing test_nwbiterator_unit_system failed intermittently in CI -- twice in recent runs, once on Windows and once on macOS, always as `assert 2000 == 3000`, one 1000-sample chunk short. It passes 25/25 locally and 12/12 under 12x CPU load, so the trigger is runner speed rather than anything reproducible here. The cause is not in this branch. `NWBIteratorUnit` yields its last chunk and immediately raises NormalTermination, which sets ezmsg's terminate event; monitor_termination then cancels every task outright. There is no drain phase, and the MessageLogger is in another process, so a message still in flight when the cancel lands is simply gone. That is a real defect in self_terminating and it predates this branch -- the test and both units it exercises are untouched here. Scaling changed the timing around it (int16 rather than float32 on the wire, one more attrs entry to serialize), which is enough to move a race but not to create one. So the test now terminates the way the clock-driven test two functions below already does: TerminateOnTotal counting messages out of the sink, with the source told not to self-terminate. That is deterministic rather than merely luckier -- MessageLogger writes and flushes each message before forwarding it, so when the count reaches the total every message is already on disk. TerminateOnTimeout is wired alongside as a backstop, not as a second termination path. Waiting for a count converts a lost message from a failure into a hang, and a hung job is a worse signal than a red one: it burns the CI timeout and says nothing. Verified by asking for four messages from a three-message stream -- the run ends in 2.1 s and the assertions report 3. The assertion tightens from `len(messages) > 0` to `== n_messages`, since the count is now known exactly. This makes CI green. It does not fix the underlying shutdown race, and it no longer covers it; a source that self-terminates can still drop trailing messages, which is worth raising with ezmsg core.
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.
An NWB
TimeSeriesstores integer samples plus the factors that turn them into the unit it declares:This package read
dataand ignored the other three. Nothing raises, because counts and volts are both just numbers — but any downstream stage with an absolute threshold (a rail guard, a power clip) is comparing against the wrong scale, and a chain fed from a live source and one fed from a recording of that same source silently disagree by the gain.The split
The reader describes; one transformer applies.
NWBSlicerstill returns the samples the file stores — unchanged fromdev— but every message now carries what they mean:NWBScalingUnitacts on that, anywhere in the graph:Keeping conversion out of the reader is both cleaner and marginally faster. Measured end-to-end on a 5.7 GB recording at 1 s chunks: 4.15 ms/chunk converting downstream vs 4.43 ms/chunk converting during the read, over a 4.06 ms floor for reading int16 and not converting. The graph moves int16 until something actually needs volts, and only the messages that survive get widened.
Not a breaking change
The reader returns exactly what
devreturns. This PR is purely additive to its behaviour: messages gainattrs["nwb_scaling"], and conversion is one line you opt into.iterator.pyandclockdriven.pyare byte-identical todev;slicer.pyis +51 lines.The trade-off is that a naive read still yields counts. That is smaller than it looks: the failure mode being removed was counts labelled volts, and now nothing claims a unit until something has made it true.
attrs["unit"]appears only after the transformer runs.NWBScalingSettingsconversion_dtypefloat32)scale_overrideunit_overridetarget_unitVoltageUnitEach accepts a bare value or a
{stream_key: value}mapping keyed onmsg.key. They layer deliberately: the overrides establish what the file actually holds, thentarget_unitconverts from that to what you want. Sounit_override="volts", target_unit="microvolts"is a net 1e6, where believing the file's own"microvolts"label would have been a no-op.Why the unit is read off h5py, not pynwb
unitis a fixed field onElectricalSeriesin the NWB schema, so pynwb answers"volts"for every electrical series regardless of the string on disk. Believing it is a factor of 1e6.Neither the declared unit nor the conversion can be trusted in general. All four of these have been seen on files describing the same hardware at the same 0.25 µV/count:
(0.25, microvolts),(1.0, volts, channel_conversion 5e-08),(0.25, volts),(1.0, uV). The second is the dangerous one — a writing library's default gain for a different amplifier, indistinguishable by any reader from a real one. Hence the overrides.Scope of
target_unitVoltage only. A prefix change is pure arithmetic; anything crossing a dimension needs a model of the circuit and belongs in a stage that can be told about it. A stream qualifies if it is an
ElectricalSeriesor declares a voltage unit — the latter covers a*_device_tscompanion, which points at the very same HDF5 dataset as its acquisition partner but is written as a plainTimeSeries. Gating on type alone gave two scales for the same bytes. Thevoltageflag is decided at read time and carried, because anElectricalSeriesthat stamped no unit is voltage by schema and that fact doesn't survive the trip off the file.The offset scales with the gain: it is additive in the same unit as the values, so converting only the gain would leave a non-zero-offset stream wrong by a power of ten — and looking converted.
Idempotence and errors
The transformer acts on the
appliedflag, never on the dtype, so two in a chain cannot double-scale. Inferring "has this been scaled?" from whether the data looks integral is the same silent guessing this PR removes.Raises rather than guessing: an unrecognized declared unit (pointing at
unit_override), ascale_override/unit_overrideaimed at a message already scaled upstream, and a per-channel gain whose length no longer fits the data because a stage dropped channels.Performance
Per message, 256 ch int16 @ 30 kHz, after caching a resolved plan per stream:
The largest single cost was
is_identity_scalingat 1.59 µs —np.all(np.asarray(gain) == 1.0)on a Python float, two array allocations to answer what==answers. The plan cache is per stream key, since one reader publishes every stream on one output and a single cached plan would thrash on interleaved streams.Tests
test_scaling.pycovers what the reader reports;test_convert.pycovers conversion, checked against the NWB definition directly rather than against a second implementation. The fixture makes all three factors non-trivial — a non-unitconversion, a non-zerooffset, and achannel_conversionwhose entries differ — since each is separately easy to drop and a 1.0/0.0 value can't tell you whether it was applied. It also carries a voltageTimeSeriesand apixelsone to pin both halves of the voltage gate.Also fixes a latent bug:
NWBSlicer.closereadself._iodirectly, so__del__on an object whose__init__raised turned the real error into anAttributeErrorfrom a destructor.246 tests pass; ruff clean. Verified against a real 5.7 GB recording.
🤖 Generated with Claude Code