Skip to content

Make conversion and offset required on TimeSeries children with required units - #2258

Open
h-mayorquin wants to merge 3 commits into
devfrom
make_conversion_and_offset_strict
Open

h-mayorquin wants to merge 3 commits into
devfrom
make_conversion_and_offset_strict

Conversation

@h-mayorquin

@h-mayorquin h-mayorquin commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Motivation

ElectricalSeries, CurrentClampSeries, CurrentClampStimulusSeries, VoltageClampSeries and VoltageClampStimulusSeries fix the unit of their data, but until now a user could create one without saying anything about calibration and PyNWB would quietly write conversion=1.0 and offset=0.0. An ElectricalSeries built from raw int16 samples then states that the integer 17 is 17 volts.

These five types now warn when they are created without a conversion or an offset, and that will raise an error in PyNWB 4.4.0. Passing channel_conversion counts as passing a conversion, since a per-channel scaling already carries the calibration, and that exemption is why the check belongs here rather than in the schema, which cannot express a dependency between the two fields. I am unsure on whether we should raise and when the warning should become a raise, I am open for suggestions. I picked this as a compromise between producing wrong data (which pushes for urgency and raise now) but give some air to people that already have scripts running (which would push for version 5.0).

TimeSeries now declares conversion and offset with a default of None and resolves them to the old 1.0 and 0.0 at the top of its __init__, which is what lets the five subtypes tell an explicit 1.0 apart from an unspecified one. Because that resolution happens inside the constructor, the container never carries a None, so a file produced by unchanged user code is identical to before and the only other visible effect is that conversion=None is now accepted on any TimeSeries instead of raising a type error. This was just for an easy implementation of the behavior above on the first paragraph. Maybe there is a better one.

How to test the behavior?

import numpy as np
from pynwb.ecephys import ElectricalSeries
from pynwb.testing.mock.ecephys import mock_electrodes

electrodes = mock_electrodes(n_electrodes=2)
data = np.arange(20, dtype="int16").reshape(10, 2)

ElectricalSeries(name="es", data=data, electrodes=electrodes, rate=30000.0)
# UserWarning: ElectricalSeries 'es': conversion and offset not specified. Data in 'volts' is computed as
# data * conversion + offset, so leaving conversion and offset unspecified claims that the values in data
# are already in 'volts'. This will raise an error in PyNWB 4.4.0. Specify the conversion and offset to
# remove this warning.

# silent, the calibration is given
ElectricalSeries(name="es", data=data, electrodes=electrodes, rate=30000.0, conversion=1.95e-7, offset=0.0)

# silent, a per-channel conversion already carries the calibration
ElectricalSeries(name="es", data=data, electrodes=electrodes, rate=30000.0, channel_conversion=[1.0, 2.0], offset=0.0)

Fix #2257, and see NeurodataWithoutBorders/nwb-schema#671 and NeurodataWithoutBorders/nwb-schema#670 for the schema side of this.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running ruff check . && codespell from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.19%. Comparing base (b5c3b81) to head (9232923).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2258      +/-   ##
==========================================
+ Coverage   96.17%   96.19%   +0.02%     
==========================================
  Files          30       30              
  Lines        3003     3021      +18     
  Branches      437      442       +5     
==========================================
+ Hits         2888     2906      +18     
  Misses         65       65              
  Partials       50       50              
Flag Coverage Δ
integration 75.17% <100.00%> (+0.14%) ⬆️
unit 86.49% <100.00%> (+0.08%) ⬆️

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.

@h-mayorquin
h-mayorquin requested a review from rly September 1, 2026 19:45
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.

TimeSeries subtypes with a fixed unit should not default conversion and offset

1 participant