From 8c4c877f0f47cb4a7ad6f8e58f90a590ac6181fb Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Fri, 16 Dec 2022 03:15:27 -0800 Subject: [PATCH 1/8] Added NWBHDF5IO.nwb_version property and check for version on NWBHDF5IO.read --- src/pynwb/__init__.py | 49 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/src/pynwb/__init__.py b/src/pynwb/__init__.py index 77cb74b26..2d350797e 100644 --- a/src/pynwb/__init__.py +++ b/src/pynwb/__init__.py @@ -211,6 +211,7 @@ class NWBHDF5IO(_HDF5IO): def __init__(self, **kwargs): path, mode, manager, extensions, load_namespaces, file_obj, comm, driver =\ popargs('path', 'mode', 'manager', 'extensions', 'load_namespaces', 'file', 'comm', 'driver', kwargs) + # Define the BuildManager to use if load_namespaces: if manager is not None: warn("loading namespaces from file - ignoring 'manager'") @@ -237,8 +238,53 @@ def __init__(self, **kwargs): manager = get_manager(extensions=extensions) elif manager is None: manager = get_manager() + # Open the file super().__init__(path, manager=manager, mode=mode, file=file_obj, comm=comm, driver=driver) + @property + def nwb_version(self): + """ + Get the version tuple for the NWB file. + + NOTE: The version will be None if no data has been written yet + + :returns: Tuple with the file version or None if the version is missing + """ + # Get the version string for the NWB file + try: + nwb_version_string = self._file.attrs['nwb_version'] + # KeyError occurs when the file is empty (e.g., when creating a new file nothing has been written) + # or when the HDF5 file is not a valid NWB file + except KeyError: + return None + # Parse the version string + nwb_version = tuple([int(j) if j.isnumeric() else j + for i in nwb_version_string.split(".") + for j in i.split("-")]) + return nwb_version + + @docval(*get_docval(_HDF5IO.read), + {'name': 'skip_version_check', 'type': bool, 'doc': 'skip checking of NWB version', 'default': False}) + def read(self, **kwargs): + """ + Read the NWB file from the IO source. + + :raises : If the NWB file version is missing or not support + + :return: NWBFile container + """ + # Check that the NWB file is supported + skip_verison_check = popargs('skip_version_check', kwargs) + if not skip_verison_check: + file_version = self.nwb_version + if file_version is None: + raise TypeError("Missing NWB version in file. The file is not a valid NWB file.") + if file_version[0] < 2: + raise TypeError("NWB version %s not supported. PyNWB supports NWB files version 2 and above." % + str(file_version)) + # read the file + return super().read(**kwargs) + @docval({'name': 'src_io', 'type': HDMFIO, 'doc': 'the HDMFIO object (such as NWBHDF5IO) that was used to read the data to export'}, {'name': 'nwbfile', 'type': 'NWBFile', @@ -247,7 +293,8 @@ def __init__(self, **kwargs): {'name': 'write_args', 'type': dict, 'doc': 'arguments to pass to :py:meth:`write_builder`', 'default': None}) def export(self, **kwargs): - """Export an NWB file to a new NWB file using the HDF5 backend. + """ + Export an NWB file to a new NWB file using the HDF5 backend. If ``nwbfile`` is provided, then the build manager of ``src_io`` is used to build the container, and the resulting builder will be exported to the new backend. So if ``nwbfile`` is provided, From 333ebc5fbd1e6a9a616183db32793366be4ea6af Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Fri, 16 Dec 2022 03:15:55 -0800 Subject: [PATCH 2/8] Updated icephys tests to skip version check when writing non NWBFile container --- tests/unit/test_icephys_metadata_tables.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_icephys_metadata_tables.py b/tests/unit/test_icephys_metadata_tables.py index 75d0d157a..a111aa6fd 100644 --- a/tests/unit/test_icephys_metadata_tables.py +++ b/tests/unit/test_icephys_metadata_tables.py @@ -579,7 +579,7 @@ def test_round_trip_container_no_data(self): with NWBHDF5IO(self.path, 'w') as io: io.write(curr) with NWBHDF5IO(self.path, 'r') as io: - incon = io.read() + incon = io.read(skip_version_check=True) self.assertListEqual(incon.categories, curr.categories) for n in curr.categories: # empty columns from file have dtype int64 or float64 but empty in-memory columns have dtype object From 39df9d7750b3dd707e92006b0356fdaf4a4b783a Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Fri, 16 Dec 2022 03:16:26 -0800 Subject: [PATCH 3/8] Add tests for NWB version check on read --- tests/integration/hdf5/test_io.py | 49 ++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/tests/integration/hdf5/test_io.py b/tests/integration/hdf5/test_io.py index c49abb92a..a2f114bad 100644 --- a/tests/integration/hdf5/test_io.py +++ b/tests/integration/hdf5/test_io.py @@ -418,9 +418,56 @@ def setUp(self): def tearDown(self): remove_test_file(self.path) + def test_check_nwb_version_ok(self): + """Test that opening a current NWBFile passes the version check""" + with NWBHDF5IO(self.path, 'w') as io: + io.write(self.nwbfile) + with NWBHDF5IO(self.path, 'r') as io: + self.assertIsNotNone(io.nwb_version) + self.assertGreater(io.nwb_version[0], 1) + read_file = io.read() + self.assertContainerEqual(read_file, self.nwbfile) + + def test_check_nwb_version_missing_version(self): + """Test reading of files with missing nwb_version""" + # write the example file + with NWBHDF5IO(self.path, 'w') as io: + io.write(self.nwbfile) + # remove the version attribute + with File(self.path, mode='a') as io: + del io.attrs['nwb_version'] + # test that reading the file without a version strings fails + with self.assertRaisesWith( + TypeError, + "Missing NWB version in file. The file is not a valid NWB file."): + with NWBHDF5IO(self.path, 'r') as io: + _ = io.read() + # test that reading the file when skipping the version check works + with NWBHDF5IO(self.path, 'r') as io: + read_file = io.read(skip_version_check=True) + self.assertContainerEqual(read_file, self.nwbfile) + + def test_check_nwb_version_old_version(self): + """Test reading of files with version less than 2 """ + # write the example file + with NWBHDF5IO(self.path, 'w') as io: + io.write(self.nwbfile) + # remove the version attribute + with File(self.path, mode='a') as io: + io.attrs['nwb_version'] = "1.0.5" + # test that reading the file without a version strings fails + with self.assertRaisesWith( + TypeError, + "NWB version (1, 0, 5) not supported. PyNWB supports NWB files version 2 and above."): + with NWBHDF5IO(self.path, 'r') as io: + _ = io.read() + # test that reading the file when skipping the version check works + with NWBHDF5IO(self.path, 'r') as io: + read_file = io.read(skip_version_check=True) + self.assertContainerEqual(read_file, self.nwbfile) + def test_round_trip_with_path_string(self): """Opening a NWBHDF5IO with a path string should work correctly""" - path_str = self.path with NWBHDF5IO(path_str, 'w') as io: io.write(self.nwbfile) From f351732822d912df34f6266c03e2a57c4900cc67 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Fri, 16 Dec 2022 03:29:01 -0800 Subject: [PATCH 4/8] Add unit tests for NWBHDF5IO.nwb_version property --- tests/integration/hdf5/test_io.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/integration/hdf5/test_io.py b/tests/integration/hdf5/test_io.py index a2f114bad..b4d58d65c 100644 --- a/tests/integration/hdf5/test_io.py +++ b/tests/integration/hdf5/test_io.py @@ -418,6 +418,30 @@ def setUp(self): def tearDown(self): remove_test_file(self.path) + def test_nwb_version_property(self): + """Test reading of files with missing nwb_version""" + # check empty version before write + with NWBHDF5IO(self.path, 'w') as io: + self.assertIsNone(io.nwb_version) + # write the example file + with NWBHDF5IO(self.path, 'w') as io: + io.write(self.nwbfile) + # check behavior for various different version strings + for ver in [("2.0.5", (2, 0, 5)), + ("2.0.5-alpha", (2, 0, 5, "alpha")), + ("bad_version", ("bad_version", ))]: + # Set version string + with File(self.path, mode='a') as io: + io.attrs['nwb_version'] = ver[0] + # Assert expected result for nwb_version tuple + with NWBHDF5IO(self.path, 'r') as io: + self.assertTupleEqual(io.nwb_version, ver[1]) + # check empty version attribute + with File(self.path, mode='a') as io: + del io.attrs['nwb_version'] + with NWBHDF5IO(self.path, 'r') as io: + self.assertIsNone(io.nwb_version) + def test_check_nwb_version_ok(self): """Test that opening a current NWBFile passes the version check""" with NWBHDF5IO(self.path, 'w') as io: From b1ac5c4d3c7aa8a89f777757ee5f9efce10154d9 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Fri, 16 Dec 2022 03:36:54 -0800 Subject: [PATCH 5/8] Updated changelog --- CHANGELOG.md | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b5a878bd..d3f014d36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,26 +6,28 @@ - `Subject.age` can be input as a `timedelta`. @bendichter [#1590](https://github.com/NeurodataWithoutBorders/pynwb/pull/1590) - `IntracellularRecordingsTable.add_recording`: the `electrode` arg is now optional, and is automatically populated from the stimulus or response. [#1597](https://github.com/NeurodataWithoutBorders/pynwb/pull/1597) -- Add module `pynwb.testing.mock.icephys` and corresponding tests. @bendichter +- Added module `pynwb.testing.mock.icephys` and corresponding tests. @bendichter [1595](https://github.com/NeurodataWithoutBorders/pynwb/pull/1595) -- Remove redundant object mapper code. @rly [#1600](https://github.com/NeurodataWithoutBorders/pynwb/pull/1600) -- Fix pending deprecations and issues in CI. @rly [#1594](https://github.com/NeurodataWithoutBorders/pynwb/pull/1594) +- Removed redundant object mapper code. @rly [#1600](https://github.com/NeurodataWithoutBorders/pynwb/pull/1600) +- Fixed pending deprecations and issues in CI. @rly [#1594](https://github.com/NeurodataWithoutBorders/pynwb/pull/1594) +- Added ``NWBHDF5IO.nwb_version`` property to get the NWB version from an NWB HDF5 file @oruebel [#1612](https://github.com/NeurodataWithoutBorders/pynwb/pull/1612) +- Updated ``NWBHDF5IO.read`` to check NWB version before read and raise more informative error if an unsupported version is found @oruebel [#1612](https://github.com/NeurodataWithoutBorders/pynwb/pull/1612) ### Documentation and tutorial enhancements: - Adjusted [ecephys tutorial](https://pynwb.readthedocs.io/en/stable/tutorials/domain/ecephys.html) to create fake data with proper dimensions @bendichter [#1581](https://github.com/NeurodataWithoutBorders/pynwb/pull/1581) - Refactored testing documentation, including addition of section on ``pynwb.testing.mock`` submodule. @bendichter [#1583](https://github.com/NeurodataWithoutBorders/pynwb/pull/1583) -- Update round trip tutorial to the newer ``NWBH5IOMixin`` and ``AcquisitionH5IOMixin`` classes. @bendichter +- Updated round trip tutorial to the newer ``NWBH5IOMixin`` and ``AcquisitionH5IOMixin`` classes. @bendichter [#1586](https://github.com/NeurodataWithoutBorders/pynwb/pull/1586) -- More informative error message for common installation error. @bendichter, @rly +- Added more informative error message for common installation error. @bendichter, @rly [#1591](https://github.com/NeurodataWithoutBorders/pynwb/pull/1591) -- Update citation for PyNWB in docs and duecredit to use the eLife NWB paper. @oruebel [#1604](https://github.com/NeurodataWithoutBorders/pynwb/pull/1604) -- Fix docs build warnings due to use of hardcoded links. @oruebel [#1604](https://github.com/NeurodataWithoutBorders/pynwb/pull/1604) +- Updated citation for PyNWB in docs and duecredit to use the eLife NWB paper. @oruebel [#1604](https://github.com/NeurodataWithoutBorders/pynwb/pull/1604) +- Fixed docs build warnings due to use of hardcoded links. @oruebel [#1604](https://github.com/NeurodataWithoutBorders/pynwb/pull/1604) ### Bug fixes -- Add shape constraint to `PatchClampSeries.data`. @bendichter +- Added shape constraint to `PatchClampSeries.data`. @bendichter [#1596](https://github.com/NeurodataWithoutBorders/pynwb/pull/1596) -- Update the [images tutorial](https://pynwb.readthedocs.io/en/stable/tutorials/domain/images.html) to provide example usage of an ``IndexSeries`` +- Updated the [images tutorial](https://pynwb.readthedocs.io/en/stable/tutorials/domain/images.html) to provide example usage of an ``IndexSeries`` with a reference to ``Images``. @bendichter [#1602](https://github.com/NeurodataWithoutBorders/pynwb/pull/1602) - Fixed an issue with the `tox` tool when upgrading to tox 4. @rly [#1608](https://github.com/NeurodataWithoutBorders/pynwb/pull/1608) From fd779e4aa0675d3736be0b0434513979e161b52a Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Fri, 16 Dec 2022 03:50:05 -0800 Subject: [PATCH 6/8] Fixed docstring for NWBHDF5IO.read --- src/pynwb/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pynwb/__init__.py b/src/pynwb/__init__.py index 2d350797e..38f864d54 100644 --- a/src/pynwb/__init__.py +++ b/src/pynwb/__init__.py @@ -269,7 +269,7 @@ def read(self, **kwargs): """ Read the NWB file from the IO source. - :raises : If the NWB file version is missing or not support + :raises TypeError: If the NWB file version is missing or not support :return: NWBFile container """ From 06b6d56343bd7fd56804170d940ad28cbea873e6 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Fri, 16 Dec 2022 18:02:12 -0800 Subject: [PATCH 7/8] Fix spelling --- src/pynwb/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/pynwb/__init__.py b/src/pynwb/__init__.py index 38f864d54..ad7b40ca7 100644 --- a/src/pynwb/__init__.py +++ b/src/pynwb/__init__.py @@ -246,9 +246,9 @@ def nwb_version(self): """ Get the version tuple for the NWB file. - NOTE: The version will be None if no data has been written yet + NOTE: The version will be None if no data has been written yet. - :returns: Tuple with the file version or None if the version is missing + :returns: Tuple with the file version or None if the version is missing. """ # Get the version string for the NWB file try: @@ -269,13 +269,13 @@ def read(self, **kwargs): """ Read the NWB file from the IO source. - :raises TypeError: If the NWB file version is missing or not support + :raises TypeError: If the NWB file version is missing or not supported :return: NWBFile container """ # Check that the NWB file is supported - skip_verison_check = popargs('skip_version_check', kwargs) - if not skip_verison_check: + skip_version_check = popargs('skip_version_check', kwargs) + if not skip_version_check: file_version = self.nwb_version if file_version is None: raise TypeError("Missing NWB version in file. The file is not a valid NWB file.") From 297606743fcf52aa2793958e0e61f2c28c560f28 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Fri, 16 Dec 2022 23:13:27 -0800 Subject: [PATCH 8/8] Update NWBHDF5IO.nwb_version to return the version as both a string and tuple --- src/pynwb/__init__.py | 19 +++++++++++-------- tests/integration/hdf5/test_io.py | 17 ++++++++++------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/pynwb/__init__.py b/src/pynwb/__init__.py index 38f864d54..53fb7f93d 100644 --- a/src/pynwb/__init__.py +++ b/src/pynwb/__init__.py @@ -248,7 +248,10 @@ def nwb_version(self): NOTE: The version will be None if no data has been written yet - :returns: Tuple with the file version or None if the version is missing + :returns: Tuple consisting of: 1) the original version string and 2) a tuple with the parsed + components of the version string, containing ints and strings, e.g., (2, 5, 1, beta). + (None, None) will be returned if the nwb_version is missing, e.g., when no data has + been written to the file yet. """ # Get the version string for the NWB file try: @@ -256,12 +259,12 @@ def nwb_version(self): # KeyError occurs when the file is empty (e.g., when creating a new file nothing has been written) # or when the HDF5 file is not a valid NWB file except KeyError: - return None + return None, None # Parse the version string - nwb_version = tuple([int(j) if j.isnumeric() else j - for i in nwb_version_string.split(".") - for j in i.split("-")]) - return nwb_version + nwb_version_parts = nwb_version_string.replace("-", ".").replace("_", ".").split(".") + nwb_version = tuple([int(i) if i.isnumeric() else i + for i in nwb_version_parts]) + return nwb_version_string, nwb_version @docval(*get_docval(_HDF5IO.read), {'name': 'skip_version_check', 'type': bool, 'doc': 'skip checking of NWB version', 'default': False}) @@ -276,12 +279,12 @@ def read(self, **kwargs): # Check that the NWB file is supported skip_verison_check = popargs('skip_version_check', kwargs) if not skip_verison_check: - file_version = self.nwb_version + file_version_str, file_version = self.nwb_version if file_version is None: raise TypeError("Missing NWB version in file. The file is not a valid NWB file.") if file_version[0] < 2: raise TypeError("NWB version %s not supported. PyNWB supports NWB files version 2 and above." % - str(file_version)) + str(file_version_str)) # read the file return super().read(**kwargs) diff --git a/tests/integration/hdf5/test_io.py b/tests/integration/hdf5/test_io.py index b4d58d65c..8009faa74 100644 --- a/tests/integration/hdf5/test_io.py +++ b/tests/integration/hdf5/test_io.py @@ -422,33 +422,36 @@ def test_nwb_version_property(self): """Test reading of files with missing nwb_version""" # check empty version before write with NWBHDF5IO(self.path, 'w') as io: - self.assertIsNone(io.nwb_version) + self.assertTupleEqual(io.nwb_version, (None, None)) # write the example file with NWBHDF5IO(self.path, 'w') as io: io.write(self.nwbfile) # check behavior for various different version strings for ver in [("2.0.5", (2, 0, 5)), ("2.0.5-alpha", (2, 0, 5, "alpha")), - ("bad_version", ("bad_version", ))]: + ("1.0.4_beta", (1, 0, 4, "beta")), + ("bad_version", ("bad", "version", ))]: # Set version string with File(self.path, mode='a') as io: io.attrs['nwb_version'] = ver[0] # Assert expected result for nwb_version tuple with NWBHDF5IO(self.path, 'r') as io: - self.assertTupleEqual(io.nwb_version, ver[1]) + self.assertEqual(io.nwb_version[0], ver[0]) + self.assertTupleEqual(io.nwb_version[1], ver[1]) # check empty version attribute with File(self.path, mode='a') as io: del io.attrs['nwb_version'] with NWBHDF5IO(self.path, 'r') as io: - self.assertIsNone(io.nwb_version) + self.assertTupleEqual(io.nwb_version, (None, None)) def test_check_nwb_version_ok(self): """Test that opening a current NWBFile passes the version check""" with NWBHDF5IO(self.path, 'w') as io: io.write(self.nwbfile) with NWBHDF5IO(self.path, 'r') as io: - self.assertIsNotNone(io.nwb_version) - self.assertGreater(io.nwb_version[0], 1) + self.assertIsNotNone(io.nwb_version[0]) + self.assertIsNotNone(io.nwb_version[1]) + self.assertGreater(io.nwb_version[1][0], 1) read_file = io.read() self.assertContainerEqual(read_file, self.nwbfile) @@ -482,7 +485,7 @@ def test_check_nwb_version_old_version(self): # test that reading the file without a version strings fails with self.assertRaisesWith( TypeError, - "NWB version (1, 0, 5) not supported. PyNWB supports NWB files version 2 and above."): + "NWB version 1.0.5 not supported. PyNWB supports NWB files version 2 and above."): with NWBHDF5IO(self.path, 'r') as io: _ = io.read() # test that reading the file when skipping the version check works