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

Add support for NWB schema 2.3.0 #1245

Merged
merged 14 commits into from
May 18, 2021
Merged

Add support for NWB schema 2.3.0 #1245

merged 14 commits into from
May 18, 2021

Conversation

rly
Copy link
Contributor

@rly rly commented Jun 1, 2020

The current release of PyNWB, version 1.3.2, supports nwb-schema version 2.2.5. This PR adds support for nwb-schema minor version 2.3.0.

This PR tracks the dev branch of nwb-schema to ensure that changes in nwb-schema are compatible with PyNWB or are accompanied with corresponding changes in PyNWB. Those changes in PyNWB should be merged into this branch.

  • make waveforms doubly indexed
  • Update PyNWB to work with changes in nwb-schema:
    • Add optional waveforms column to the Units table.
    • Add optional strain field to Subject.
    • Add to DecompositionSeries an optional DynamicTableRegion called source_channels.
    • Add to ImageSeries an optional link to Device.
    • Add optional continuity field to TimeSeries.
    • Add optional filtering attribute to ElectricalSeries.
    • Clarify documentation for electrode impedance and filtering.
    • Add description of extra fields.
    • Set the stimulus_description for IZeroCurrentClamp to have the fixed value N/A.
    • Update hdmf-common-schema from 1.1.3 to version 1.5.0.
      • The HDMF-experimental namespace was added, which includes the ExternalResources and EnumData
        data types. Schema in the HDMF-experimental namespace are experimental and subject to breaking changes at any time.
      • Added experimental data type ExternalResources for storing ontology information / external resource references.
      • Added experimental data type EnumData to store data from a set of fixed values.
      • Changed dtype for datasets within CSRMatrix from 'int' to 'uint' and added missing data_type_inc: Container
        to the CSRMatrix type.
      • Added data type SimpleMultiContainer, a Container for storing other Container and Data objects together.
      • Added data type AlignedDynamicTable, a DynamicTable type with support for categories (or sub-headings) each described by a separate DynamicTable.
      • Fixed missing dtype for VectorIndex.
      • VectorIndex now extends VectorData instead of Index.
      • Removed unused and non-functional Index data type.
  • Run validation tests
  • Update changelog
  • After new version of nwb-schema is released, update the nwb-schema submodule to point to latest release: cd src/pynwb/nwb-schema/, git checkout dev, git pull origin dev, git fetch --tags, git checkout dev, cd ../../.., git add ., git commit -m "Update nwb-schema submodule to [new_release], and git push

After merging:

  • This branch should be merged into dev without rebased commits, NOT squashed commits.

@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #1245 (13b6c4e) into dev (130665a) will decrease coverage by 0.00%.
The diff coverage is 73.07%.

❗ Current head 13b6c4e differs from pull request most recent head 50eb5b9. Consider uploading reports for the commit 50eb5b9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1245      +/-   ##
==========================================
- Coverage   73.41%   73.41%   -0.01%     
==========================================
  Files          37       37              
  Lines        2310     2328      +18     
  Branches      354      357       +3     
==========================================
+ Hits         1696     1709      +13     
- Misses        553      557       +4     
- Partials       61       62       +1     
Impacted Files Coverage Δ
src/pynwb/base.py 92.85% <ø> (ø)
src/pynwb/io/base.py 30.50% <0.00%> (-0.53%) ⬇️
src/pynwb/ophys.py 95.13% <ø> (ø)
src/pynwb/validate.py 0.00% <0.00%> (ø)
src/pynwb/misc.py 87.31% <60.00%> (+0.09%) ⬆️
src/pynwb/file.py 82.13% <66.66%> (-0.41%) ⬇️
src/pynwb/ecephys.py 96.46% <100.00%> (+0.03%) ⬆️
src/pynwb/icephys.py 90.08% <100.00%> (+0.60%) ⬆️
src/pynwb/image.py 94.44% <100.00%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 130665a...50eb5b9. Read the comment docs.

@rly
Copy link
Contributor Author

rly commented May 13, 2021

Only validation errors remain before this is ready to merge. (This is not tested on per-PR CI, but nightly CI.)

The main error is:

ValueError: data type 'NWBFile' not found in namespace hdmf-experimental

hdmf-experimental is cached within the file but NWB objects should not be validated against it.

@rly rly marked this pull request as ready for review May 18, 2021 00:31
@rly rly force-pushed the schema_2.3.0 branch 2 times, most recently from e1ebed7 to 1b19251 Compare May 18, 2021 00:47
bendichter and others added 13 commits May 17, 2021 17:53
* remove validate_core_schema.

You can check the core schema with e.g.

* Update Subject unit test

* Fix Subject unit test, add check for date_of_birth

Co-authored-by: Ryan Ly <[email protected]>
* add option to include source_channels for DecompositionSeries and include round trip test
* Use latest schema
* Add unit test
* Add optional link to Device in ImageSeries

* Add support for device in ImageSeries subclasses

* Update ImageMaskSeries docstring

* Update schema
* add continuity optional arg to TimeSeries.__init__

* use new enum feature of hdmf to check against controlled vocabulary

* Fix flake8

* Add integration test

* Fix flake8

* Add correct mapping for data.continuity

Co-authored-by: Ryan Ly <[email protected]>
requirements-doc.txt Outdated Show resolved Hide resolved
@@ -55,7 +56,8 @@ class Subject(NWBContainer):
{'name': 'subject_id', 'type': str, 'doc': 'a unique identifier for the subject', 'default': None},
{'name': 'weight', 'type': str, 'doc': 'the weight of the subject', 'default': None},
{'name': 'date_of_birth', 'type': datetime, 'default': None,
'doc': 'datetime of date of birth. May be supplied instead of age.'})
'doc': 'datetime of date of birth. May be supplied instead of age.'},
{'name': 'strain', 'type': str, 'doc': 'the strain of the subject', 'default': None})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a recommendation for how the strain should be specified. If so, it would be nice to add an example in the 'doc' .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this. Please review.

src/pynwb/file.py Outdated Show resolved Hide resolved
src/pynwb/misc.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.

I added a few minor comments, but otherwise this looks good.

@rly rly force-pushed the schema_2.3.0 branch 2 times, most recently from fff47eb to 13b6c4e Compare May 18, 2021 05:45
@rly rly requested a review from oruebel May 18, 2021 05:48
oruebel
oruebel previously approved these changes May 18, 2021
oruebel
oruebel previously approved these changes May 18, 2021
@rly rly merged commit 2934fb0 into dev May 18, 2021
@rly rly deleted the schema_2.3.0 branch May 18, 2021 17:49
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.

3 participants