From a69d9c1586af66aa1e716f8710d73536a3476c8b Mon Sep 17 00:00:00 2001 From: rly <310197+rly@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:17:49 -0700 Subject: [PATCH] Make Units.waveform_unit configurable 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) --- CHANGELOG.md | 5 ++ docs/gallery/domain/ecephys.py | 14 ++++++ src/pynwb/io/misc.py | 30 ++++++++---- src/pynwb/misc.py | 6 ++- src/pynwb/nwb-schema | 2 +- tests/integration/hdf5/test_misc.py | 76 ++++++++++++++++++++++++++++- tests/unit/test_misc.py | 4 ++ 7 files changed, 123 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbcf1f3e4..af1e01c49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,12 @@ ## PyNWB 4.1.1 (Unreleased) +### Changed +- Added support for NWB Schema 2.10.1 + - The `unit` attribute of `Units.waveform_mean`, `Units.waveform_sd`, and `Units.waveforms` now has a default value of `"volts"` instead of a fixed value of `"volts"`. + ### Fixed +- Fixed `Units.waveform_unit` having no effect on the written file. The `waveform_unit` passed to `Units` is now written to the `waveform_mean`, `waveform_sd`, and `waveforms` columns, and `"volts"` remains the default. PyNWB now also warns when the waveform columns of a file being read carry different `unit` or `sampling_rate` attributes, since only one value per attribute is kept on the `Units` container. @rly [#2162](https://github.com/NeurodataWithoutBorders/pynwb/issues/2162) - Fixed reading a file whose dates carry a sub-minute UTC offset (e.g. `1900-10-01T00:00:00-05:50:36`). @h-mayorquin [#2230](https://github.com/NeurodataWithoutBorders/pynwb/pull/2230) - Fixed wide pandas DataFrames in the tutorials spilling out of the content column and into the right margin. @bendichter [#2236](https://github.com/NeurodataWithoutBorders/pynwb/pull/2236) diff --git a/docs/gallery/domain/ecephys.py b/docs/gallery/domain/ecephys.py index ad3da8b24..1437daa0a 100644 --- a/docs/gallery/domain/ecephys.py +++ b/docs/gallery/domain/ecephys.py @@ -358,6 +358,20 @@ # The :py:class:`~pynwb.misc.Units` table can contain simply the spike times of sorted units, or you can also include # individual and mean waveform information in some of the optional, predefined :py:class:`~pynwb.misc.Units` table # columns: ``waveform_mean``, ``waveform_sd``, or ``waveforms``. +# +# The sampling rate and unit of measurement of those three columns are set with the ``waveform_rate`` and +# ``waveform_unit`` arguments of :py:class:`~pynwb.misc.Units`, which default to ``None`` and ``"volts"``. +# Because both are constructor arguments, set them when you build the :py:class:`~pynwb.misc.Units` table and +# assign it to :py:attr:`.NWBFile.units`:: +# +# from pynwb.misc import Units +# +# nwbfile.units = Units( +# name="units", +# description="units table", +# waveform_rate=30000.0, +# waveform_unit="microvolts", +# ) nwbfile.units.to_dataframe() diff --git a/src/pynwb/io/misc.py b/src/pynwb/io/misc.py index f5d39b5ec..3593a82d6 100644 --- a/src/pynwb/io/misc.py +++ b/src/pynwb/io/misc.py @@ -1,3 +1,5 @@ +import warnings + from hdmf.common.io.table import DynamicTableMap from .. import register_map @@ -22,17 +24,27 @@ def waveform_unit_carg(self, builder, manager): return self._get_waveform_stat(builder, 'unit') def _get_waveform_stat(self, builder, attribute): + """Get the value of an attribute shared by the waveform columns of a Units table. + + The `Units` container holds one `waveform_rate` and one `waveform_unit` for the whole table, while the + file stores a `sampling_rate` and `unit` attribute on each waveform column. When the columns disagree, + the value of the first populated column is used and a warning is raised. + """ waveform_columns = ('waveform_mean', 'waveform_sd', 'waveforms') - stats = [builder[column].attributes.get(attribute) for column in waveform_columns if column in builder] - if not stats: + stats = {column: builder[column].attributes.get(attribute) + for column in waveform_columns if column in builder} + populated_stats = {column: value for column, value in stats.items() if value is not None} + if not populated_stats: return None - populated_stats = [stat for stat in stats if stat is not None] - if len(set(populated_stats)) > 1: - # throw warning - pass - if populated_stats: - return populated_stats[0] - return None + first_column, first_value = next(iter(populated_stats.items())) + if len(set(populated_stats.values())) > 1: + warnings.warn( + f"The '{attribute}' attribute differs across the waveform columns of Units " + f"'{builder.name}': {populated_stats}. Using the value of '{first_column}'.", + UserWarning, + stacklevel=2 + ) + return first_value @DynamicTableMap.object_attr("electrodes") def electrodes_column(self, container, manager): diff --git a/src/pynwb/misc.py b/src/pynwb/misc.py index a0549b914..c19cacecc 100644 --- a/src/pynwb/misc.py +++ b/src/pynwb/misc.py @@ -180,9 +180,11 @@ class Units(DynamicTable): {'name': 'electrode_table', 'type': DynamicTable, 'doc': 'the table that the *electrodes* column indexes', 'default': None}, {'name': 'waveform_rate', 'type': float, - 'doc': 'Sampling rate of the waveform means', 'default': None}, + 'doc': 'Sampling rate of the data in the waveform_mean, waveform_sd, and waveforms columns', + 'default': None}, {'name': 'waveform_unit', 'type': str, - 'doc': 'Unit of measurement of the waveform means', 'default': 'volts'}, + 'doc': 'Unit of measurement of the data in the waveform_mean, waveform_sd, and waveforms columns', + 'default': 'volts'}, {'name': 'resolution', 'type': float, 'doc': 'The smallest possible difference between two spike times', 'default': None}, allow_positional=AllowPositional.WARNING, diff --git a/src/pynwb/nwb-schema b/src/pynwb/nwb-schema index eed08cf84..cff37d314 160000 --- a/src/pynwb/nwb-schema +++ b/src/pynwb/nwb-schema @@ -1 +1 @@ -Subproject commit eed08cf842727eac156ad165abdc2aaea59b757f +Subproject commit cff37d3140db777b6cbe10ddb920b00766d0da9e diff --git a/tests/integration/hdf5/test_misc.py b/tests/integration/hdf5/test_misc.py index b07066a62..9ca656394 100644 --- a/tests/integration/hdf5/test_misc.py +++ b/tests/integration/hdf5/test_misc.py @@ -2,9 +2,10 @@ import numpy as np from hdmf.common import VectorData, DynamicTableRegion -from pynwb import TimeSeries +from pynwb import NWBHDF5IO, TimeSeries from pynwb.misc import Units, DecompositionSeries, FrequencyBandsTable -from pynwb.testing import NWBH5IOMixin, AcquisitionH5IOMixin, TestCase +from pynwb.testing import NWBH5IOMixin, AcquisitionH5IOMixin, TestCase, remove_test_file +from pynwb.testing.mock.file import mock_NWBFile from pynwb.ecephys import ElectrodeGroup, ElectrodesTable from pynwb.device import Device @@ -123,6 +124,77 @@ def test_waveforms_attributes_written(self): self.assertEqual(unit, 'volts') +class TestUnitsCustomWaveformUnitIO(AcquisitionH5IOMixin, TestCase): + """Test roundtripping a waveform unit other than the default 'volts'.""" + + def setUpContainer(self): + ut = Units( + name='UnitsCustomWaveformUnitTest', + description='a simple table for testing a custom Units waveform unit', + waveform_rate=40000., + waveform_unit='microvolts', + ) + ut.add_unit( + spike_times=[0., 1., 2.], + waveform_mean=[1., 2., 3.], + waveform_sd=[4., 5., 6.], + waveforms=[ + [ # elec 1 + [1, 2, 3], + [1, 2, 3] + ], [ # elec 2 + [1, 2, 3], + [1, 2, 3] + ] + ], + ) + return ut + + def test_waveform_unit_roundtrip(self): + ut = self.roundtripContainer() + self.assertEqual(ut.waveform_unit, 'microvolts') + + def test_waveform_unit_written(self): + self.roundtripContainer() + with h5py.File(self.filename, 'r') as infile: + units = infile['acquisition'][self.container.name] + for column in ('waveform_mean', 'waveform_sd', 'waveforms'): + unit = units[column].attrs['unit'] + if isinstance(unit, bytes): + unit = unit.decode('utf-8') + self.assertEqual(unit, 'microvolts') + + +class TestUnitsMismatchedWaveformUnit(TestCase): + """Test reading a file whose waveform columns carry different unit attributes.""" + + def setUp(self): + self.filename = 'test_units_mismatched_waveform_unit.nwb' + nwbfile = mock_NWBFile() + ut = Units(name='units', description='a table for testing mismatched waveform units') + ut.add_unit( + spike_times=[0., 1., 2.], + waveform_mean=[1., 2., 3.], + waveform_sd=[4., 5., 6.], + ) + nwbfile.units = ut + with NWBHDF5IO(self.filename, 'w') as io: + io.write(nwbfile) + with h5py.File(self.filename, 'r+') as infile: + infile['units']['waveform_sd'].attrs['unit'] = 'microvolts' + + def tearDown(self): + remove_test_file(self.filename) + + def test_warn_on_mismatched_unit(self): + msg = ("The 'unit' attribute differs across the waveform columns of Units 'units': " + "{'waveform_mean': 'volts', 'waveform_sd': 'microvolts'}. Using the value of 'waveform_mean'.") + with self.assertWarnsWith(UserWarning, msg): + with NWBHDF5IO(self.filename, 'r') as io: + nwbfile = io.read() + self.assertEqual(nwbfile.units.waveform_unit, 'volts') + + class TestUnitsFileIO(NWBH5IOMixin, TestCase): def setUpContainer(self): diff --git a/tests/unit/test_misc.py b/tests/unit/test_misc.py index 5cbe1dffa..a114b5bed 100644 --- a/tests/unit/test_misc.py +++ b/tests/unit/test_misc.py @@ -323,6 +323,10 @@ def test_waveform_attrs(self): self.assertEqual(ut.waveform_rate, 40000.) self.assertEqual(ut.waveform_unit, 'volts') + def test_custom_waveform_unit(self): + ut = Units(waveform_unit='microvolts') + self.assertEqual(ut.waveform_unit, 'microvolts') + def test_get_starting_time(self): """Test get_starting_time returns the earliest spike time across units.""" ut = Units()