Avoid raising data timestamps length mismatch warning with external file (#1486)

* extend check criteria of data-timestamps mismatch warming

* add unittest to check new behavior

* move check to private method

* overwrite timeseries check method

* remove unused import

* remove unused import

* trigger time series dimension warning when external_file is None

* change dimension check to class method

* refactor tests

* flake8

* Update to fix property calls and add docs

* Fix check

* Fix check

* Fix check

* Fix test with warning

* update CHANGELOG.md

* test warning is not raised with rate specified

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

Author: weiglszonja

CHANGELOG.md

@@ -25,6 +25,8 @@
 - Added support for HDMF 3.3.1. This is now the minimum version of HDMF supported. Importantly, HDMF 3.3 introduces
   warnings when the constructor of a class mapped to an HDMF-common data type or an autogenerated data type class
   is passed positional arguments instead of all keyword arguments. @rly (#1484)
+- Moved logic that checks the 0th dimension of TimeSeries data equals the length of timestamps to a private method in the
+  ``TimeSeries`` class. This is to avoid raising a warning when an ImageSeries is used with external file. @weiglszonja (#1486)
 
 ### Documentation and tutorial enhancements:
 - Added tutorial on annotating data via ``TimeIntervals``. @oruebel (#1390)

src/pynwb/base.py

@@ -166,24 +166,6 @@ def __init__(self, **kwargs):
         for key, val in args_to_set.items():
             setattr(self, key, val)
 
-        data_shape = get_data_shape(data=args_to_process["data"], strict_no_data_load=True)
-        timestamps_shape = get_data_shape(data=args_to_process["timestamps"], strict_no_data_load=True)
-        if (
-            # check that the shape is known
-            data_shape is not None and timestamps_shape is not None
-
-            # check for scalars. Should never happen
-            and (len(data_shape) > 0 and len(timestamps_shape) > 0)
-
-            # check that the length of the first dimension is known
-            and (data_shape[0] is not None and timestamps_shape[0] is not None)
-
-            # check that the data and timestamps match
-            and (data_shape[0] != timestamps_shape[0])
-        ):
-            warn("Length of data does not match length of timestamps. Your data may be transposed. Time should be on "
-                 "the 0th dimension")
-
         data = args_to_process['data']
         self.fields['data'] = data
         if isinstance(data, TimeSeries):
@@ -207,6 +189,29 @@ def __init__(self, **kwargs):
         else:
             raise TypeError("either 'timestamps' or 'rate' must be specified")
 
+        if not self._check_time_series_dimension():
+            warn("Length of data does not match length of timestamps. Your data may be transposed. Time should be on "
+                 "the 0th dimension")
+
+    def _check_time_series_dimension(self):
+        """Check that the 0th dimension of data equals the length of timestamps, when applicable.
+        """
+        if self.timestamps is None:
+            return True
+
+        data_shape = get_data_shape(data=self.fields["data"], strict_no_data_load=True)
+        timestamps_shape = get_data_shape(data=self.fields["timestamps"], strict_no_data_load=True)
+
+        # skip check if shape of data or timestamps cannot be computed
+        if data_shape is None or timestamps_shape is None:
+            return True
+
+        # skip check if length of the first dimension is not known
+        if data_shape[0] is None or timestamps_shape[0] is None:
+            return True
+
+        return data_shape[0] == timestamps_shape[0]
+
     @property
     def num_samples(self):
         ''' Tries to return the number of data samples. If this cannot be assessed, returns None.

src/pynwb/image.py

@@ -72,14 +72,36 @@ def __init__(self, **kwargs):
         if unit is None:
             kwargs['unit'] = ImageSeries.DEFAULT_UNIT
 
-        # TODO catch warning when default data is used and timestamps are provided
         super().__init__(**kwargs)
 
         if args_to_set["external_file"] is None:
             args_to_set["starting_frame"] = None  # overwrite starting_frame
         for key, val in args_to_set.items():
             setattr(self, key, val)
 
+        if not self._check_image_series_dimension():
+            warnings.warn(
+                "Length of data does not match length of timestamps. "
+                "Your data may be transposed. Time should be on the 0th dimension"
+            )
+
+    def _check_time_series_dimension(self):
+        """Override _check_time_series_dimension to do nothing.
+        The _check_image_series_dimension method will be called instead.
+        """
+        return True
+
+    def _check_image_series_dimension(self):
+        """Check that the 0th dimension of data equals the length of timestamps, when applicable.
+
+        ImageSeries objects can have an external file instead of data stored. The external file cannot be
+        queried for the number of frames it contains, so this check will return True when an external file
+        is provided. Otherwise, this function calls the parent class' _check_time_series_dimension method.
+        """
+        if self.external_file is not None:
+            return True
+        return super()._check_time_series_dimension()
+
     @property
     def bits_per_pixel(self):
         return self.fields.get('bits_per_pixel')

tests/integration/hdf5/test_ophys.py

@@ -140,7 +140,7 @@ class TestTwoPhotonSeriesIO(AcquisitionH5IOMixin, TestCase):
     def setUpContainer(self):
         """ Return the test TwoPhotonSeries to read/write """
         self.device, self.optical_channel, self.imaging_plane = make_imaging_plane()
-        data = [[[1., 1.] * 2] * 2]
+        data = np.ones((10, 2, 2))
         timestamps = list(map(lambda x: x/10, range(10)))
         fov = [2.0, 2.0, 5.0]
         ret = TwoPhotonSeries(

tests/unit/test_image.py

@@ -1,3 +1,5 @@
+import warnings
+
 import numpy as np
 
 from pynwb import TimeSeries
@@ -73,6 +75,48 @@ def test_external_file_no_unit(self):
         )
         self.assertEqual(iS.unit, ImageSeries.DEFAULT_UNIT)
 
+    def test_dimension_warning(self):
+        """Test that a warning is raised when the dimensions of the data are not the
+        same as the dimensions of the timestamps."""
+        msg = (
+            "Length of data does not match length of timestamps. Your data may be "
+            "transposed. Time should be on the 0th dimension"
+        )
+        with self.assertWarnsWith(UserWarning, msg):
+            ImageSeries(
+                name='test_iS',
+                data=np.ones((3, 3, 3)),
+                unit='Frames',
+                starting_frame=[0],
+                timestamps=[1, 2, 3, 4]
+            )
+
+    def test_dimension_warning_external_file_with_timestamps(self):
+        """Test that a warning is not raised when external file is used with timestamps."""
+        with warnings.catch_warnings(record=True) as w:
+            ImageSeries(
+                name='test_iS',
+                external_file=['external_file'],
+                format='external',
+                unit='Frames',
+                starting_frame=[0],
+                timestamps=[1, 2, 3, 4]
+            )
+            self.assertEqual(w, [])
+
+    def test_dimension_warning_external_file_with_rate(self):
+        """Test that a warning is not raised when external file is used with rate."""
+        with warnings.catch_warnings(record=True) as w:
+            ImageSeries(
+                name='test_iS',
+                external_file=['external_file'],
+                format='external',
+                unit='Frames',
+                starting_frame=[0],
+                rate=0.2,
+            )
+            self.assertEqual(w, [])
+
 
 class IndexSeriesConstructor(TestCase):