Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support stimulus_template as optional column in IntracellularRecordingsTable #1815

Merged
merged 16 commits into from
Jan 12, 2024

Conversation

stephprince
Copy link
Contributor

@stephprince stephprince commented Jan 9, 2024

Motivation

Fix #1747 to support stimulus_template as a predefined optional column of the IntracellularStimuliTable
(See NeurodataWithoutBorders/nwb-schema#545 for the related schema PR)

Since the proposed column neurodata type is TimeSeriesReferenceVectorData, I though it was maybe unclear how to add data to the column so I added an example in the tutorials.

The IntracellularRecordingsTable.add_recording had some nice functionality to automatically compute the start_index and count values for stimulus time series inputs, as well as handle recordings with no stimulus info provided. I added similar behavior for stimulus_template, but I'm not sure if that's something that would be helpful or if it's best to add additional stimulus template data via the stimulus_metadata argument

How to test the behavior?

from pynwb.base import TimeSeriesReferenceVectorData
from pynwb.testing.mock.file import mock_NWBFile
from pynwb.testing.mock.icephys import (mock_IntracellularRecordingsTable,
                                        mock_VoltageClampStimulusSeries,
                                        mock_IntracellularElectrode,)

# adding data by row
nwbfile = mock_NWBFile()
electrode = mock_IntracellularElectrode(nwbfile=nwbfile)
stimulus = mock_VoltageClampStimulusSeries(nwbfile=nwbfile)
nwbfile.add_intracellular_recording(electrode=electrode,
                                    stimulus=stimulus,
                                    stimulus_template=stimulus)

# adding data by column
irt = mock_IntracellularRecordingsTable()
irt.add_column(name='stimulus_template',
               description='',
               category='stimuli',
               col_cls=TimeSeriesReferenceVectorData,
               data=[(0, 5, stimulus),
                     (0, 5, stimulus),
                     (0, 5, stimulus),
                     (0, 5, stimulus),
                     (0, 5, stimulus),])

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 flake8 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.

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d0c4068) 91.99% compared to head (a7d1f56) 92.02%.

Additional details and impacted files
@@                 Coverage Diff                  @@
##           nwb-schema-2.7.0    #1815      +/-   ##
====================================================
+ Coverage             91.99%   92.02%   +0.03%     
====================================================
  Files                    27       27              
  Lines                  2623     2633      +10     
  Branches                685      690       +5     
====================================================
+ Hits                   2413     2423      +10     
  Misses                  138      138              
  Partials                 72       72              
Flag Coverage Δ
integration 70.94% <30.00%> (-0.16%) ⬇️
unit 83.74% <100.00%> (+0.06%) ⬆️

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

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

@stephprince stephprince marked this pull request as ready for review January 10, 2024 23:30
@stephprince stephprince requested a review from oruebel January 10, 2024 23:30
src/pynwb/base.py Outdated Show resolved Hide resolved
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

Looks overall good to me. I added a couple of comments for the tutorial part but those should be easy to fix.

@oruebel
Copy link
Contributor

oruebel commented Jan 11, 2024

Looks good to me. @rly is doing a final review an will approve once ready.

src/pynwb/base.py Outdated Show resolved Hide resolved
src/pynwb/base.py Show resolved Hide resolved
@rly
Copy link
Contributor

rly commented Jan 11, 2024

@oruebel because you requested changes earlier, you still need to approve this

@rly rly changed the base branch from dev to nwb-schema-2.7.0 January 11, 2024 22:51
@stephprince stephprince merged commit 6a5420b into nwb-schema-2.7.0 Jan 12, 2024
23 checks passed
@stephprince stephprince deleted the add-stimulus-template-column branch January 12, 2024 00:03
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.

[Feature]: Support stimulus_template column in IntracellularRecordingsTable
3 participants