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

Fix integration tests with invalid test data that will be caught by future hdmf validator version #1366

Conversation

dsleiter
Copy link
Contributor

@dsleiter dsleiter commented Jun 9, 2021

Fix integration tests with invalid test data that will begin to fail with an update to hdmf validator that increases validation coverage.

See the discussion here: hdmf-dev/hdmf#609 (comment)

Motivation

Integration tests which use TestElectricalSeriesIO.make_electrode_table are failing in the pynwb CI tests for hdmf-dev/hdmf#609. That PR is fixing a validator issue which caused the validator to not validate certain builders. With that fix, the validator is now correctly validating many builders which were not previously being validated, including in unit tests.

This PR resolves item 3 in the discussion for hdmf PR#609

The issue is that when adding rows to an electrode table in make_electrode_table, the value for x in table.add_row is provided as an int while the schema expects a float.

How to test the behavior?

  1. checkout the dev branch for pynwb
  2. checkout and pip install the hdmf branch from this PR: Validate builders against both top level data type specs and inner specs hdmf-dev/hdmf#609
  3. execute the ecephys integration tests (not using tox, since tox would manage the hdmf dependency):
$ python -m unittest tests/integration/hdf5/test_ecephys.py 

you should see 13 test failures with the following message:

Exception: root/general/extracellular_ephys/electrodes/x (general/extracellular_ephys/electrodes/x): incorrect type - expected 'float32', got 'int64'
  1. checkout the pynwb branch for this PR (or just manually apply the changes from here) and rerun the same tests. 13 tests should fail again, however this time they should fail for a different reason:
Exception: root/general/extracellular_ephys/electrodes/filtering (general/extracellular_ephys/electrodes/filtering): incorrect type - expected 'float32', got 'utf'

This failure is due to item 4 the discussion for hdmf PR#609, and is due to what appears to be a disagreement between nwb-schema and pynwb, and will not be fixed just by unit tests.

Note: I understand we can't merge in failing tests. However, with the current hdmf version these tests will continue to pass. HDMF PR#609 won't be merged in until we resolve the discrepancy in the dtype of the electrode table filtering column, so these changes are safe to merge in here now.

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.

* These unit tests will begin to fail after the update to the hdmf
  validator that increase validation coverage
* Ref hdmf-dev/hdmf#585
* Ref hdmf-dev/hdmf#609
* See discussion: hdmf-dev/hdmf#609 (comment)
@t-b t-b self-requested a review June 9, 2021 08:41
t-b
t-b previously approved these changes Jun 9, 2021
oruebel
oruebel previously approved these changes Jun 9, 2021
@dsleiter dsleiter dismissed stale reviews from oruebel and t-b via c7e6bd8 June 23, 2021 02:34
rly
rly previously approved these changes Jun 25, 2021
@rly rly merged commit f656778 into NeurodataWithoutBorders:dev Jun 25, 2021
@dsleiter dsleiter deleted the bug/fix_tests_for_validator_update branch June 25, 2021 01:48
@rly rly mentioned this pull request Jul 2, 2021
6 tasks
rly pushed a commit that referenced this pull request Aug 10, 2021
Declare SweepTable as deprecated on init

Add TimeSeriesReferenceVectorData Container class

Add simple test for TimeSeriesReferenceVectorData

Update nwb-schema branch

Add icephys metadata container classes

Update NWBFile to support icephys metadata tables

Add unit tests for icephys metadata tables

Update imports and fix SweepTable integration tests

Fix test available namespaces testcase

Update SweepTable tutorial and declare as deprecated

Add new icephys tutorial using the new hierarchical metadata tables

Add IntracellularRecordingsTable illustration

Fix flake8 issues

Remove use of HierarchiclaDynamicTableMixin use for now

Add source PowerPoint file for icephys docs

add doc for TimeIntervals (#1352)

* add doc for TimeIntervals

* add allensdk back

Fix make clean command for docs to also clean sphinx-gallery files

Updated CHANGELOG to describe changes from this PR

Add thumbnails for domain-specfic tutorials

Add thumbnaisl for general tutorials

Update changelog

Remove ic_filtering/icephys_filtering duplicate and fix test

Fix description of ExperimentalConditionsTable

Use HDMF 2.5.0 (#1354)

DOC: make template less puzzling in how to fill out ;-) (#1336)

Co-authored-by: Ryan Ly <[email protected]>

Add ReadTheDocs badge to README (#1311)

Co-authored-by: Ryan Ly <[email protected]>

Update to use upcoming HDMF 2.5.1 (#1355)

Revert breaking changes to scratch API (#1356)

Use HDMF 2.5.2 (#1360)

Add `DfOverF` to docs (#1351)

Co-authored-by: Ryan Ly <[email protected]>

docs/gallery/general/file.py: Fix argument count in documentation

add strain optional arg to Subject (#1241)

* 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 source channels to DecompSeries

add option to include source_channels for DecompositionSeries (#1258)

* 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 (#1265)

* 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__ (#1226)

* 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]>

Add support for ElectricalSeries.filtering dataset (#1270)

Set fixed value for IZeroClampSeries stimulus description

Fix test

Update changelog

Add note about allensdk py3.8 incompatibility

Add workaround for validation against cached hdmf-experimental

Update CI to avoid click8 incompatibility

Use HDMF 2.5.5

Address comments, improve doc

Fix docs, add citation info, update repo infra (#1362)

Use latest HDMF, raise min pandas, dissociate req-min and setup reqs (#1363)

Use nwb schema branch for icephys meta

Ensure start_index and count are set to -1 if TimeSeries is None

Add test for conversion of start_index and index_count vals

Fix flake8 in __compute_index docstring

Fix icephys metadata roundtrip NWBFile test base on update DynamicTableRegion behavior

Update schema (updated version to 1.4.0-alpha)

improve extensions documentation (#1350)

* improve extensions documentation

* adjustments to extensions.rst

* add tips to extension docs

* create multi-part extensions tutorial

* move API building docs to chapter 5 of extensions guide

* restructure and fix up some spots

* Update docs/source/extensions_tutorial/2_create_extension_spec_walkthrough.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/2_create_extension_spec_walkthrough.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/2_create_extension_spec_walkthrough.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/6_documenting_extension.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/4_auto_api.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Oliver Ruebel <[email protected]>

* remove detailed GroupSpec API instructions from tutorial

* remove instructions for generated the directory structure, and instead focus on generating the HTML and filling in the missing pieces.

* add link to ndx catalog

* Update docs/source/extensions_tutorial/2_create_extension_spec_walkthrough.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/2_create_extension_spec_walkthrough.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/2_create_extension_spec_walkthrough.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/4_auto_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/5_custom_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/4_auto_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/4_auto_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/4_auto_api.rst

* Update docs/source/extensions_tutorial/3_spec_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/5_custom_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update docs/source/extensions_tutorial/5_custom_api.rst

Co-authored-by: Ryan Ly <[email protected]>

* Update extensions_tutorial_home.rst

* Minor text fixes

* Minor text fixes

* Minor text fixes

Co-authored-by: Oliver Ruebel <[email protected]>
Co-authored-by: Ryan Ly <[email protected]>

Set language for code block to fix readthedocs

Add RRID for PyNWB

Add RRID to README

Update changelog

make MotionCorrection test (#1374)

* attempt to make MotionCorrection test. I don't understand why it's failing

* Fix CorrectedImageStack nwbfields

* Fix tests

* Fix test

* Fix flake8

* Fix test

Co-authored-by: Ryan Ly <[email protected]>

Add unique electrode id check (#1344)

Co-authored-by: Ryan Ly <[email protected]>

Fix integration tests with invalid test data that will be caught by future hdmf validator version (#1366)

Co-authored-by: Ryan Ly <[email protected]>

Improve documentation and test for CorrectedImageStack (#1306)

Fix test for validator update (#1376)

Update changelog

Allow h5py 3 so that this can work with hdmf rc/3.0.0

Move SweepTable DeprectationWarning to the constructor of SweepTable to clean up code

Suppress SweepTable Deprecation warning in icephys integration test

Ignore and assert deprecation warning for SweepTable during SweepTable roundtrip tests

Merge branch 'rc/2.0.0' into add/icephys_meta

Fix falke8 on icephys tutorial galleries
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.

4 participants