Repository: NeurodataWithoutBorders/pynwb

Issue:
[Bug]: Timeseries can have timestamps + data that don't match in length
### What happened?

I use the convenient timeseries pointer feature, where you can just store timestamps in one timeseries and have many others point to them. This saves a lot of memory! However, I just fixed a bug that arose because some data I stored in a timeseries didn't have the same length as the timeseries (due to a processing mistake). Then I went and checked, and it turns out that timeseries' timestamps don't need to have the same length as the data at all! I consider this a bug, given that timeseries are meant as a core unit of data in NWB, but let me know if I'm just using them wrong.

### Steps to Reproduce

```python
# normal case
t = nwb.TimeSeries(name='test', data=np.arange(10), timestamps=np.arange(10), unit='sec')

# I expect a value error here
nwb.TimeSeries(name='test', data=np.arange(10), timestamps=np.arange(5), unit='sec')

# This similarly should raise an error imo
s = nwb.TimeSeries(name='test2', data=np.arange(2), timestamps=t, unit='sec')
```


### Traceback

```python
n/a
```


### Operating System

macOS

### Python Executable

Conda

### Python Version

3.9

### Package Versions

nwb 2.0.0

### Code of Conduct

- [X] I agree to follow this project's [Code of Conduct](https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/.github/CODE_OF_CONDUCT.rst)
- [X] Have you checked the [Contributing](https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/docs/CONTRIBUTING.rst) document?
- [X] Have you ensured this bug was not already [reported](https://github.com/NeurodataWithoutBorders/pynwb/issues)?

Discussion:
bendichter: @weiglszonja can you please resolve these conflicts?
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1538?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 82.29%. Comparing base [(`739ee54`)](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/commit/739ee548374cd4bc5de5e339069f02778733f86a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) to head [(`a62dca5`)](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/commit/a62dca55b191edb64ca2ec2126e8ec7174d12eb8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders).
> Report is 1 commits behind head on dev.

> :exclamation:  There is a different number of reports uploaded between BASE (739ee54) and HEAD (a62dca5). Click for more details.
> 
> <details><summary>HEAD has 7 uploads less than BASE</summary>
>
>| Flag | BASE (739ee54) | HEAD (a62dca5) |
>|------|------|------|
>|unit|4|1|
>|integration|4|0|
></details>

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##              dev    #1538      +/-   ##
==========================================
- Coverage   91.73%   82.29%   -9.45%     
==========================================
  Files          27       27              
  Lines        2722     2722              
  Branches      710      709       -1     
==========================================
- Hits         2497     2240     -257     
- Misses        149      382     +233     
- Partials       76      100      +24     
```

| [Flag](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1538/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1538/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | `?` | |
| [unit](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1538/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders) | `82.29% <100.00%> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders#carryforward-flags-in-the-pull-request-comment) to find out more.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/NeurodataWithoutBorders/pynwb/pull/1538?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NeurodataWithoutBorders).

h-mayorquin: > @weiglszonja
> 
> This code stores images in an ImageSeries so that they can be referenced by IndexSeries: https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/docs/gallery/domain/brain_observatory.py#L71-L85

@rly 
I looked at the data and they are in fact Videos:

![image](https://github.com/user-attachments/assets/d99774d7-8ce5-4554-9ba5-6a041fe6d856)

So I don't know how the Images, order of images, and friends would work very well here. 

h-mayorquin: @rly @bendichter 
Looking at the inspector tests it looks that it fails because it can't create some of the files that the inspector requires on its testing. This makes sense as this PR makes that impossible. How to handle this?
rly: > @rly @bendichter Looking at the inspector tests it looks that it fails because it can't create some of the files that the inspector requires on its testing. This makes sense as this PR makes that impossible. How to handle this?

We can create the test files in other ways in the nwbinspector. I'll create an issue/PR to adjust that.
rly: @bendichter @stephprince It is strange that the brain observatory example uses the allensdk API to download an NWB file into a cache folder and then creates a new NWB file with the same data step by step. Do we want to maintain this example? I think everything that is demonstrated here is demonstrated in other tutorials (aside from the allensdk API).

This example also causes a minor headache with the test harness because it involves installing allensdk which is not well maintained with new versions of python, and it downloads a 450 MB file, which takes some time.
bendichter: @rly I would be in support of removing that tutorial
bendichter: Or at least demoting it to some sore of 3rd party untested status
stephprince: @rly I'm also in support of removing that example
stephprince: I think the allensdk API is demonstrated much more in depth in [their documentation](https://allensdk.readthedocs.io/en/latest/brain_observatory.html) and the pynwb examples are all demonstrated in other tutorials like you mentioned.
rly: I opened #2026 which we can merge first to remove the Brain Observatory example.

Patch:
diff --git a/src/pynwb/base.py b/src/pynwb/base.py
index 8b4daf48..6b64b3ee 100644
--- a/src/pynwb/base.py
+++ b/src/pynwb/base.py
@@ -199,28 +199,34 @@ class TimeSeries(NWBDataInterface):
         else:
             raise TypeError("either 'timestamps' or 'rate' must be specified")
 
-        if not self._check_time_series_dimension():
-            warn("%s '%s': Length of data does not match length of timestamps. Your data may be transposed. "
-                 "Time should be on the 0th dimension" % (self.__class__.__name__, self.name))
+        self._error_on_new_warn_on_construct(
+            error_msg=self._check_time_series_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
+            return
 
         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
+            return
 
         # 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
 
-        return data_shape[0] == timestamps_shape[0]
+        if data_shape[0] == timestamps_shape[0]:
+            return
+
+        return (
+            "%s '%s': Length of data does not match length of timestamps. Your data may be transposed. "
+            "Time should be on the 0th dimension" % (self.__class__.__name__, self.name)
+        )
 
     @property
     def num_samples(self):
diff --git a/src/pynwb/image.py b/src/pynwb/image.py
index c775297d..84f445ea 100644
--- a/src/pynwb/image.py
+++ b/src/pynwb/image.py
@@ -102,13 +102,9 @@ class ImageSeries(TimeSeries):
                 DeprecationWarning,
             )
 
-        if not self._check_image_series_dimension():
-            warnings.warn(
-                "%s '%s': Length of data does not match length of timestamps. Your data may be transposed. "
-                "Time should be on the 0th dimension"
-                % (self.__class__.__name__, self.name)
-            )
-
+        self._error_on_new_warn_on_construct(
+            error_msg=self._check_image_series_dimension()
+        )
         self._error_on_new_warn_on_construct(
             error_msg=self._check_external_file_starting_frame_length()
         )
@@ -135,7 +131,7 @@ class ImageSeries(TimeSeries):
         """Override _check_time_series_dimension to do nothing.
         The _check_image_series_dimension method will be called instead.
         """
-        return True
+        return
 
     def _check_image_series_dimension(self):
         """Check that the 0th dimension of data equals the length of timestamps, when applicable.
@@ -145,7 +141,7 @@ class ImageSeries(TimeSeries):
         is provided. Otherwise, this function calls the parent class' _check_time_series_dimension method.
         """
         if self.external_file is not None:
-            return True
+            return
         return super()._check_time_series_dimension()
 
     def _check_external_file_starting_frame_length(self):


Test patch:
diff --git a/tests/unit/test_base.py b/tests/unit/test_base.py
index 5af4986a..3fcaa1d0 100644
--- a/tests/unit/test_base.py
+++ b/tests/unit/test_base.py
@@ -375,12 +375,13 @@ class TestTimeSeries(TestCase):
                 timestamps=[0.3, 0.4, 0.5],
             )
 
-    def test_dimension_warning(self):
+    def test_timestamps_data_length_error_raised(self):
+        """Test that TimeSeries cannot be created with timestamps and data of different lengths."""
         msg = (
             "TimeSeries 'test_ts2': 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):
+        with self.assertRaisesWith(ValueError, msg):
             TimeSeries(
                 name="test_ts2",
                 data=[10, 11, 12],
@@ -478,6 +479,39 @@ class TestTimeSeries(TestCase):
         pm.add(ts2)
         self.assertIn('(link to processing/test_ts1/timestamps)', pm._repr_html_())
 
+    def test_timestamps_data_length_warning_construct_mode(self):
+        """
+        Test that warning is raised when the length of data does not match the length of
+        timestamps in case that the TimeSeries in construct mode (i.e., during read).
+        """
+        msg = (
+            "TimeSeries 'test_ts2': Length of data does not match length of timestamps. Your data may be "
+            "transposed. Time should be on the 0th dimension"
+        )
+        for timestamps in [[0], [1, 2, 3, 4]]:
+            with self.subTest():
+                # Create the time series in construct mode, modelling the behavior
+                # of the ObjectMapper on read while avoiding having to create, write,
+                # and read and entire NWB file
+                obj = TimeSeries.__new__(
+                    TimeSeries,
+                    container_source=None,
+                    parent=None,
+                    object_id="test",
+                    in_construct_mode=True,
+                )
+                with self.assertWarnsWith(UserWarning, msg):
+                    obj.__init__(
+                        name="test_ts2",
+                        data=[10, 11, 12],
+                        unit="grams",
+                        timestamps=timestamps,
+                    )
+                # Disable construct mode. Since we are not using this object anymore
+                # this is not strictly necessary but is good style in case we expand
+                # the test later on.
+                obj._in_construct_mode = False
+
 
 class TestImage(TestCase):
     def test_init(self):
diff --git a/tests/unit/test_image.py b/tests/unit/test_image.py
index 92637d82..3f8b56b5 100644
--- a/tests/unit/test_image.py
+++ b/tests/unit/test_image.py
@@ -84,14 +84,13 @@ class ImageSeriesConstructor(TestCase):
         )
         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."""
+    def test_dimension_error(self):
+        """Test that ImageSeries cannot be created with timestamps and data of different lengths."""
         msg = (
             "ImageSeries 'test_iS': 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):
+        with self.assertRaisesWith(ValueError, msg):
             ImageSeries(
                 name='test_iS',
                 data=np.ones((3, 3, 3)),
@@ -100,9 +99,14 @@ class ImageSeriesConstructor(TestCase):
             )
 
     def test_dimension_warning_external_file_with_timestamps(self):
-        """Test that a warning is not raised when external file is used with timestamps."""
+        """Test that warning is not raised when external file is used with timestamps."""
+        obj = ImageSeries.__new__(ImageSeries,
+                                  container_source=None,
+                                  parent=None,
+                                  object_id="test",
+                                  in_construct_mode=True)
         with warnings.catch_warnings(record=True) as w:
-            ImageSeries(
+            obj.__init__(
                 name='test_iS',
                 external_file=['external_file'],
                 format='external',
@@ -111,6 +115,10 @@ class ImageSeriesConstructor(TestCase):
                 timestamps=[1, 2, 3, 4]
             )
             self.assertEqual(w, [])
+        # Disable construct mode. Since we are not using this object any more
+        # this is not strictly necessary but is good style in case we expand
+        # the test later on
+        obj._in_construct_mode = False
 
     def test_dimension_warning_external_file_with_rate(self):
         """Test that a warning is not raised when external file is used with rate."""