From a71e6b35abdff92ba1d1f1667270baeabc3d1cc5 Mon Sep 17 00:00:00 2001 From: rly Date: Thu, 19 Jan 2023 00:15:56 -0800 Subject: [PATCH 1/4] Add new test utility class --- src/pynwb/testing/__init__.py | 2 +- src/pynwb/testing/testh5io.py | 187 +++++++++++++++++++++++++++++++++- 2 files changed, 183 insertions(+), 6 deletions(-) diff --git a/src/pynwb/testing/__init__.py b/src/pynwb/testing/__init__.py index e017f4a16..35ae60872 100644 --- a/src/pynwb/testing/__init__.py +++ b/src/pynwb/testing/__init__.py @@ -1,5 +1,5 @@ from hdmf.testing import TestCase, H5RoundTripMixin -from .testh5io import NWBH5IOMixin, AcquisitionH5IOMixin +from .testh5io import NWBH5IOMixin, AcquisitionH5IOMixin, NWBH5IOFlexMixin from .utils import remove_test_file from .icephys_testutils import create_icephys_stimulus_and_response, create_icephys_testfile diff --git a/src/pynwb/testing/testh5io.py b/src/pynwb/testing/testh5io.py index 3627e3ad0..7346c76d3 100644 --- a/src/pynwb/testing/testh5io.py +++ b/src/pynwb/testing/testh5io.py @@ -38,13 +38,10 @@ def setUp(self): self.container_type = self.container.__class__.__name__ self.filename = 'test_%s.nwb' % self.container_type self.export_filename = 'test_export_%s.nwb' % self.container_type - self.writer = None self.reader = None self.export_reader = None def tearDown(self): - if self.writer is not None: - self.writer.close() if self.reader is not None: self.reader.close() if self.export_reader is not None: @@ -85,9 +82,14 @@ def test_roundtrip_export(self): def roundtripContainer(self, cache_spec=False): """Add the Container to an NWBFile, write it to file, read the file, and return the Container from the file. """ - description = 'a file to test writing and reading a %s' % self.container_type + session_description = 'a file to test writing and reading a %s' % self.container_type identifier = 'TEST_%s' % self.container_type - nwbfile = NWBFile(description, identifier, self.start_time, file_create_date=self.create_date) + nwbfile = NWBFile( + session_description=session_description, + identifier=identifier, + session_start_time=self.start_time, + file_create_date=self.create_date + ) self.addContainer(nwbfile) with warnings.catch_warnings(record=True) as ws: @@ -199,3 +201,178 @@ def addContainer(self, nwbfile): def getContainer(self, nwbfile): ''' Get the NWBDataInterface object from the file ''' return nwbfile.get_acquisition(self.container.name) + + +class NWBH5IOFlexMixin(metaclass=ABCMeta): + """ + Mixin class that includes methods to run a flexible roundtrip test. + The setUp, test_roundtrip, and tearDown methods will be run by unittest. + + The abstract methods getContainerType, addContainer, and getContainer must be + implemented by classes that includethis mixin. + + Example:: + + class TestMyContainerIO(NWBH5IOFlexMixin, TestCase): + def getContainerType(self): + # return the name of the type of Container being tested, for test ID purposes + def addContainer(self): + # add the test Container to the test NWB file + def getContainer(self, nwbfile): + # return the test Container from an NWB file + + This class is more flexible than NWBH5IOMixin and should be used for new roundtrip tests. + + This code is adapted from hdmf.testing.H5RoundTripMixin. + """ + + def setUp(self): + container_type = self.getContainerType().replace(" ", "_") + session_description = 'A file to test writing and reading a %s' % container_type + identifier = 'TEST_%s' % container_type + session_start_time = datetime(1971, 1, 1, 12, tzinfo=tzutc()) + self.nwbfile = NWBFile( + session_description=session_description, + identifier=identifier, + session_start_time=session_start_time, + ) + self.addContainer() + self.container = self.getContainer(self.nwbfile) + self.filename = 'test_%s.nwb' % container_type + self.export_filename = 'test_export_%s.nwb' % container_type + self.reader = None + self.export_reader = None + + def tearDown(self): + if self.reader is not None: + self.reader.close() + if self.export_reader is not None: + self.export_reader.close() + remove_test_file(self.filename) + remove_test_file(self.export_filename) + + def getContainerType() -> str: + """Return the name of the type of Container being tested, for test ID purposes.""" + raise NotImplementedError('Cannot run test unless getContainerType is implemented.') + + @abstractmethod + def addContainer(self): + """Add the test Container to the NWBFile. + + The NWBFile is accessible from self.nwbfile. + The Container should be accessible from getContainer(self.nwbfile). + """ + raise NotImplementedError('Cannot run test unless addContainer is implemented.') + + @abstractmethod + def getContainer(self, nwbfile: NWBFile): + """Return the test Container from the given NWBFile.""" + raise NotImplementedError('Cannot run test unless getContainer is implemented.') + + def test_roundtrip(self): + """Test whether the read Container has the same contents as the original Container and validate the file. + """ + self.read_container = self.roundtripContainer() + self.assertIsNotNone(str(self.container)) # added as a test to make sure printing works + self.assertIsNotNone(str(self.read_container)) + # make sure we get a completely new object + self.assertNotEqual(id(self.container), id(self.read_container)) + # make sure the object ID is preserved + self.assertIs(self.read_nwbfile.objects[self.container.object_id], self.read_container) + self.assertContainerEqual(self.read_container, self.container) + + def test_roundtrip_export(self): + """ + Test whether the test Container read from an exported file has the same contents as the original test Container + and validate the file. + """ + self.read_container = self.roundtripExportContainer() # this container is read from the exported file + self.assertIsNotNone(str(self.read_container)) # added as a test to make sure printing works + # make sure we get a completely new object + self.assertNotEqual(id(self.container), id(self.read_container)) + # make sure the object ID is preserved + self.assertIs(self.read_exported_nwbfile.objects[self.container.object_id], self.read_container) + self.assertContainerEqual(self.read_container, self.container, ignore_hdmf_attrs=True) + + def roundtripContainer(self, cache_spec=False): + """Write the file, validate the file, read the file, and return the Container from the file. + """ + + # catch all warnings + with warnings.catch_warnings(record=True) as ws: + with NWBHDF5IO(self.filename, mode='w') as write_io: + write_io.write(self.nwbfile, cache_spec=cache_spec) + + self.validate() + + # this is not closed until tearDown() or an exception from self.getContainer below + self.reader = NWBHDF5IO(self.filename, mode='r') + self.read_nwbfile = self.reader.read() + + # parse warnings and raise exceptions for certain types of warnings + if ws: + for w in ws: + if issubclass(w.category, (MissingRequiredBuildWarning, + BrokenLinkWarning)): + raise Exception('%s: %s' % (w.category.__name__, w.message)) + else: + warnings.warn(w.message, w.category) + + try: + return self.getContainer(self.read_nwbfile) + except Exception as e: + self.reader.close() + self.reader = None + raise e + + def roundtripExportContainer(self, cache_spec=False): + """ + Roundtrip the container, then export the read NWBFile to a new file, validate the files, and return the test + Container from the file. + """ + # write and read the file. self.reader will be set + self.roundtripContainer(cache_spec=cache_spec) + + # catch all warnings + with warnings.catch_warnings(record=True) as ws: + NWBHDF5IO.export_io( + src_io=self.reader, + path=self.export_filename, + cache_spec=cache_spec, + ) + + self.validate() + + # this is not closed until tearDown() or an exception from self.getContainer below + self.export_reader = NWBHDF5IO(self.export_filename, mode='r') + self.read_exported_nwbfile = self.export_reader.read() + + # parse warnings and raise exceptions for certain types of warnings + if ws: + for w in ws: + if issubclass(w.category, (MissingRequiredBuildWarning, + BrokenLinkWarning)): + raise Exception('%s: %s' % (w.category.__name__, w.message)) + else: + warnings.warn(w.message, w.category) + + try: + return self.getContainer(self.read_exported_nwbfile) + except Exception as e: + self.export_reader.close() + self.export_reader = None + raise e + + def validate(self): + """Validate the created files.""" + if os.path.exists(self.filename): + with NWBHDF5IO(self.filename, mode='r') as io: + errors = pynwb_validate(io) + if errors: + raise Exception("\n".join(errors)) + + if os.path.exists(self.export_filename): + with NWBHDF5IO(self.filename, mode='r') as io: + errors = pynwb_validate(io) + if errors: + raise Exception("\n".join(errors)) \ No newline at end of file From dbffcfd42bbfcdf23e0ef381f0131a0089135c37 Mon Sep 17 00:00:00 2001 From: rly Date: Thu, 19 Jan 2023 00:16:09 -0800 Subject: [PATCH 2/4] Allow Images in stimulus templates --- src/pynwb/file.py | 5 ++-- tests/integration/hdf5/test_nwbfile.py | 36 +++++++++++++++++++++++++- tests/unit/test_file.py | 7 +++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/pynwb/file.py b/src/pynwb/file.py index eb2d97e5d..7bd1f452e 100644 --- a/src/pynwb/file.py +++ b/src/pynwb/file.py @@ -18,6 +18,7 @@ from .icephys import (IntracellularElectrode, SweepTable, PatchClampSeries, IntracellularRecordingsTable, SimultaneousRecordingsTable, SequentialRecordingsTable, RepetitionsTable, ExperimentalConditionsTable) +from .image import Images from .ophys import ImagingPlane from .ogen import OptogeneticStimulusSite from .misc import Units @@ -156,7 +157,7 @@ class NWBFile(MultiContainerInterface): { 'attr': 'stimulus_template', 'add': '_add_stimulus_template_internal', - 'type': TimeSeries, + 'type': (TimeSeries, Images), 'get': 'get_stimulus_template' }, { @@ -838,7 +839,7 @@ def add_stimulus(self, **kwargs): if use_sweep_table: self._update_sweep_table(timeseries) - @docval({'name': 'timeseries', 'type': TimeSeries}, + @docval({'name': 'timeseries', 'type': (TimeSeries, Images)}, {'name': 'use_sweep_table', 'type': bool, 'default': False, 'doc': 'Use the deprecated SweepTable'}) def add_stimulus_template(self, **kwargs): timeseries = popargs('timeseries', kwargs) diff --git a/tests/integration/hdf5/test_nwbfile.py b/tests/integration/hdf5/test_nwbfile.py index 90c02aac5..c8d3688ef 100644 --- a/tests/integration/hdf5/test_nwbfile.py +++ b/tests/integration/hdf5/test_nwbfile.py @@ -7,10 +7,12 @@ from hdmf.common import DynamicTable from pynwb import NWBFile, TimeSeries, NWBHDF5IO, get_manager +from pynwb.base import Image, Images from pynwb.file import Subject from pynwb.epoch import TimeIntervals from pynwb.ecephys import ElectricalSeries -from pynwb.testing import NWBH5IOMixin, TestCase, remove_test_file +from pynwb.testing import NWBH5IOMixin, NWBH5IOFlexMixin, TestCase, remove_test_file +from pynwb.testing.mock.file import mock_NWBFile class TestNWBFileHDF5IO(TestCase): @@ -586,3 +588,35 @@ def test_roundtrip(self): super().test_roundtrip() for ii, item in enumerate(self.read_container): pd.testing.assert_frame_equal(self.table[ii+1], item) + + +class TestAddStimulusTemplateTimeSeries(NWBH5IOFlexMixin, TestCase): + + def getContainerType(self): + return "time series stored as a stimulus template" + + def addContainer(self): + ts = TimeSeries( + name="test_ts", + data=[0, 1, 2, 3, 4, 5], + unit="grams", + timestamps=[0.0, 0.1, 0.2, 0.3, 0.4, 0.5] + ) + self.nwbfile.add_stimulus_template(ts) + + def getContainer(self, nwbfile): + return nwbfile.get_stimulus_template("test_ts") + + +class TestAddStimulusTemplateImages(NWBH5IOFlexMixin, TestCase): + + def getContainerType(self): + return "images stored as a stimulus template" + + def addContainer(self): + image1 = Image(name="test_image1", data=np.ones((10, 10))) + images = Images(name="images_name", images=[image1]) + self.nwbfile.add_stimulus_template(images) + + def getContainer(self, nwbfile): + return nwbfile.get_stimulus_template("images_name") diff --git a/tests/unit/test_file.py b/tests/unit/test_file.py index dd1508c8b..c888e89d9 100644 --- a/tests/unit/test_file.py +++ b/tests/unit/test_file.py @@ -5,6 +5,7 @@ from dateutil.tz import tzlocal, tzutc from pynwb import NWBFile, TimeSeries, NWBHDF5IO +from pynwb.base import Image, Images from pynwb.file import Subject, ElectrodeTable from pynwb.epoch import TimeIntervals from pynwb.ecephys import ElectricalSeries @@ -153,6 +154,12 @@ def test_add_stimulus_template(self): 'grams', timestamps=[0.0, 0.1, 0.2, 0.3, 0.4, 0.5])) self.assertEqual(len(self.nwbfile.stimulus_template), 1) + def test_add_stimulus_template_images(self): + image1 = Image(name='test_image1', data=np.ones((10, 10))) + images = Images(name='images_name', images=[image1]) + self.nwbfile.add_stimulus_template(images) + self.assertEqual(len(self.nwbfile.stimulus_template), 1) + def test_add_analysis(self): self.nwbfile.add_analysis(TimeSeries('test_ts', [0, 1, 2, 3, 4, 5], 'grams', timestamps=[0.0, 0.1, 0.2, 0.3, 0.4, 0.5])) From a0c353e6109e512a501042c28e51a6db6d3a0de6 Mon Sep 17 00:00:00 2001 From: rly Date: Thu, 19 Jan 2023 00:17:17 -0800 Subject: [PATCH 3/4] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f12991684..054a8e34f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ - 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) +- Fixed an issue where `Images` were not allowed as stimulus templates. @rly [#1638](https://github.com/NeurodataWithoutBorders/pynwb/pull/1638) ## PyNWB 2.2.0 (October 19, 2022) From dababb1667428dae05b1cbe888a65936f59f3934 Mon Sep 17 00:00:00 2001 From: rly Date: Thu, 19 Jan 2023 00:17:58 -0800 Subject: [PATCH 4/4] Fix flake8 --- src/pynwb/testing/testh5io.py | 2 +- tests/integration/hdf5/test_nwbfile.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/pynwb/testing/testh5io.py b/src/pynwb/testing/testh5io.py index 7346c76d3..08626f943 100644 --- a/src/pynwb/testing/testh5io.py +++ b/src/pynwb/testing/testh5io.py @@ -375,4 +375,4 @@ def validate(self): with NWBHDF5IO(self.filename, mode='r') as io: errors = pynwb_validate(io) if errors: - raise Exception("\n".join(errors)) \ No newline at end of file + raise Exception("\n".join(errors)) diff --git a/tests/integration/hdf5/test_nwbfile.py b/tests/integration/hdf5/test_nwbfile.py index c8d3688ef..b93a697c5 100644 --- a/tests/integration/hdf5/test_nwbfile.py +++ b/tests/integration/hdf5/test_nwbfile.py @@ -12,7 +12,6 @@ from pynwb.epoch import TimeIntervals from pynwb.ecephys import ElectricalSeries from pynwb.testing import NWBH5IOMixin, NWBH5IOFlexMixin, TestCase, remove_test_file -from pynwb.testing.mock.file import mock_NWBFile class TestNWBFileHDF5IO(TestCase):