Add example of setting Units.resolution in the ecephys tutorial#2174
Add example of setting Units.resolution in the ecephys tutorial#2174h-mayorquin wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2174 +/- ##
=======================================
Coverage 95.18% 95.18%
=======================================
Files 30 30
Lines 2991 2991
Branches 444 444
=======================================
Hits 2847 2847
Misses 86 86
Partials 58 58
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # | ||
| # When creating the :py:class:`~pynwb.misc.Units` table directly, you can pass it to the constructor: | ||
| # | ||
| # .. code-block:: python | ||
| # | ||
| # units = Units(name="units", resolution=1 / 30000) # 30 kHz recording system | ||
| # | ||
| # Since the tutorial uses :py:meth:`.NWBFile.add_unit`, we set it after the fact: |
There was a problem hiding this comment.
| # | |
| # When creating the :py:class:`~pynwb.misc.Units` table directly, you can pass it to the constructor: | |
| # | |
| # .. code-block:: python | |
| # | |
| # units = Units(name="units", resolution=1 / 30000) # 30 kHz recording system | |
| # | |
| # Since the tutorial uses :py:meth:`.NWBFile.add_unit`, we set it after the fact: |
I don't think we need to show how to set it on the constructor. That is a rare edge case.
There was a problem hiding this comment.
how is that an edge case? how are you thinking about it?
There was a problem hiding this comment.
I think most users would use nwbfile.add_unit() and nwbfile.add_unit_column() as we recommend here, rather than setting nwbfile.units = Units(name="units", ...) or creating a Units object to add elsewhere. What do you think?
There was a problem hiding this comment.
We use the pattern in neuroconv:
https://github.com/catalystneuro/neuroconv/blob/2ddf1744208050a6229ad67d0e45f634253ae69e/src/neuroconv/tools/spikeinterface/spikeinterface.py#L2041-L2064
But yes, for the units table we don't realy use the Units constructor so it will somehow off.
I will remove it.
There was a problem hiding this comment.
As an aside: looking at the tutorials, the constructor is used for pretty much everything (ElectricalSeries, LFP, DecompositionSeries, etc.) except the "canonical" dynamic tables on NWBFile (units, trials, epochs, electrodes, invalid_times), which always go through the add_X / add_X_column methods. The only place the gallery constructs a TimeIntervals directly is for custom (non-canonical) interval tables like sleep_stages in plot_timeintervals.py.
That convention makes sense, but I don't think it is written down anywhere, which is why "edge case" didn't immediately click for me. I remember being confused myself about how to add a constructed Units table to an NWBFile.
Motivation
At the inspector we are working on
check_units_resolution_is_setcheck (NeurodataWithoutBorders/nwbinspector#685), which flagsUnitstables that containspike_timesbut have noresolutionset. Users running the inspector on files created by following our tutorials would get this warning with no guidance on how to fix it.I have added a short section to the ecephys tutorial that sets
nwbfile.units.resolutionright after the existingadd_unitloop, with a brief explanation of what the field means and why it matters. I kept the change minimal to avoid restructuring the existing tutorial flow.What was the reasoning behind this change? Please explain the changes briefly.
Checklist
ruff check . && codespellfrom the source directory.