Skip to content

Make Units.waveform_unit configurable - #2237

Merged
rly merged 3 commits into
devfrom
fix-2162-waveform-unit
Aug 11, 2026
Merged

Make Units.waveform_unit configurable#2237
rly merged 3 commits into
devfrom
fix-2162-waveform-unit

Conversation

@rly

@rly rly commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #2162.

Updates the nwb-schema submodule to the latest schema dev (2.10.1-alpha), which includes NeurodataWithoutBorders/nwb-schema#707 and NeurodataWithoutBorders/nwb-schema#709.

Configurable waveform unit (nwb-schema#707)

That PR changes the unit attribute of Units.waveform_mean, waveform_sd, and waveforms from a fixed value of "volts" to a default value of "volts".

HDMF short-circuits attribute resolution when a spec has a fixed value, so the existing mapping from Units.waveform_unit onto the unit attribute was never reached. With the schema change it is reachable, so no write-path change was needed.

Also implements the warning placeholder in UnitsMap._get_waveform_stat, since a file can now carry different unit values across the three waveform columns while the Units container holds only one.

Waveform peak alignment (nwb-schema#709)

That PR adds an optional time_before_peak_in_ms attribute to Units.waveform_mean, waveform_sd, and waveforms. It holds the time, in milliseconds, from the start of each waveform to the spike peak, that is, the alignment point used during spike sorting. The value is the same for every unit and every spike.

PyNWB exposes it as the waveform_time_before_peak_in_ms constructor argument and field of Units, following the existing waveform_rate and waveform_unit pattern: one value on the container, written onto all three waveform columns, and read back from the first populated column with a warning when the columns disagree.

NO_OVERRIDE in UnitsMap

The constructor argument overrides in UnitsMap returned None to signal "no override". HDMF 6.2.0 deprecates that in favor of the ObjectMapper.NO_OVERRIDE sentinel, and under HDMF 8.0 a None return will set the constructor argument to None, which would clobber the "volts" default of waveform_unit. They now return the sentinel via the existing pynwb.io.utils.NO_OVERRIDE shim.

Note

waveform_unit, waveform_rate, and waveform_time_before_peak_in_ms can only be set on the Units constructor, so they are not reachable from the nwbfile.add_unit(...) path that auto-creates the table. Worth addressing separately.

🤖 Generated with Claude Code

Update the nwb-schema submodule to the latest schema dev (2.10.1-alpha),
which includes NeurodataWithoutBorders/nwb-schema#707. That PR changes the
"unit" attribute of Units.waveform_mean, waveform_sd, and waveforms from a
fixed value of "volts" to a default value of "volts".

HDMF short-circuits attribute resolution when a spec has a fixed value, so
the existing mapping from Units.waveform_unit onto the "unit" attribute of
the waveform columns was never reached and the requested unit was silently
replaced with "volts" on write. With the schema change the mapping is
reachable, so no write-path change is needed.

Implement the warning placeholder in UnitsMap._get_waveform_stat: a file
can now carry different "unit" values across the three waveform columns
while the Units container holds only one, so report which column's value
was used.

Fixes #2162

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.25%. Comparing base (5101a86) to head (22e6492).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2237      +/-   ##
==========================================
+ Coverage   96.04%   96.25%   +0.20%     
==========================================
  Files          30       30              
  Lines        3010     3018       +8     
  Branches      437      438       +1     
==========================================
+ Hits         2891     2905      +14     
+ Misses         67       64       -3     
+ Partials       52       49       -3     
Flag Coverage Δ
integration 74.71% <100.00%> (+0.26%) ⬆️
unit 86.67% <25.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rly
rly requested a review from h-mayorquin August 1, 2026 00:19
h-mayorquin
h-mayorquin previously approved these changes Aug 3, 2026

@h-mayorquin h-mayorquin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, looks good to me.

@h-mayorquin

Copy link
Copy Markdown
Contributor

If we are improving the metadata provenance for waveforms it would be great to get this one as well (c.c. @alejoe91)

NeurodataWithoutBorders/nwb-schema#709

rly and others added 2 commits August 10, 2026 20:28
Update the nwb-schema submodule to the latest dev, which adds the
optional time_before_peak_in_ms attribute to the waveform_mean,
waveform_sd, and waveforms datasets of Units
(NeurodataWithoutBorders/nwb-schema#709).

Expose it as the waveform_time_before_peak_in_ms constructor argument
and field of Units, following the waveform_rate and waveform_unit
pattern: written onto all three waveform columns and read back from the
first populated one, with a warning when the columns disagree.

Return the NO_OVERRIDE sentinel from the UnitsMap constructor argument
overrides so that an absent attribute leaves the docval default in
place under HDMF >= 6.2.0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@h-mayorquin h-mayorquin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving quickly with this. LGTM.

@rly
rly merged commit 5942a7f into dev Aug 11, 2026
26 checks passed
@rly
rly deleted the fix-2162-waveform-unit branch August 11, 2026 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Units.waveform_unit parameter has no effect on serialized waveform_mean unit attribute

2 participants