-
Notifications
You must be signed in to change notification settings - Fork 84
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
Adding OnePhotonSeries class #1593
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1593 +/- ##
==========================================
+ Coverage 91.25% 91.78% +0.53%
==========================================
Files 25 26 +1
Lines 2515 2557 +42
Branches 477 489 +12
==========================================
+ Hits 2295 2347 +52
+ Misses 139 134 -5
+ Partials 81 76 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@rly Looks like when I updated the nwb-schema version, it triggered some failing container-type tests? Any ideas? |
@rly @oruebel @bendichter OK with #1540 in this now appears to be working, ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks in general good to me. Could you also add:
- A
mock_OnePhotonSeries
function to the mock test modulesrc/pynwb/testing/mock/ophys.py
Here the example for TwoPhotonSeries:
pynwb/src/pynwb/testing/mock/ophys.py
Line 66 in 5d8efdb
def mock_TwoPhotonSeries( - A roundtrip test for One`PhotonSeries to
tests/integration/hdf5/test_ophys.py
. Here the test for TwoPhotonSeries:
pynwb/tests/integration/hdf5/test_ophys.py
Line 138 in 5d8efdb
class TestTwoPhotonSeriesIO(AcquisitionH5IOMixin, TestCase):
Co-authored-by: Oliver Ruebel <[email protected]>
@oruebel Adding the round-trip IO integration test indeed reveals an issue with one of the new fields - the relevant piece of the traceback is
any suggestions for how to fix this? |
In particular, the schema specification for this field is actually as |
WooHoo 100% test coverage on the diff. Thanks @CodyCBakerPhD for adding support for OnePhotonSeries! @rly since this PR updates the schema, does this need to be merged with the dev branch or a different release branch? |
This should go to a new branch for schema 2.6 support. So that we try to stick with the release schedule, NWB 2.6 will be released around Jan. 5 and the corresponding PyNWB and MatNWB releases should be made around Jan. 8. |
I'll create that new branch and redirect this PR there. |
This is now good to merge @CodyCBakerPhD |
Motivation
Adding the
OnePhotonSeries
class to PyNWB, which was recently added to the schema in NeurodataWithoutBorders/nwb-schema#523