[BUG] Fix TimeSeries data does not match length of timestamps should raise an error when created

[weiglszonja]

Motivation

This is a follow up on the issue described in #1536 to prevent a TimeSeries being created with timestamps that does not match the length of data.

This PR changes TimeSeries _check_time_series_dimension check to warn when reading from file and raise an error when constructing new data.

This change also affects ImageSeries check that will also raise a ValueError when an ImageSeries is being created with timestamps that do not match the length of data. If ImageSeries is used with external file, then this error or warning will not be triggered.

Resolve #1536

How to test the behavior?

tests/unit/test_base.py
tests/unit/test_image.py

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.

[weiglszonja]

@oruebel
In docs/gallery/domain/brain_observatory.py ImageSeries are being created with timestamps of [0], I tried to fix it but I don't have currently access to the data that is being used here to that was my guess. But it's not working yet.

[rly]

@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

This approach is no longer the recommended approach. Instead, use the Images and Image types with IndexSeries which was recently updated to support this. For example:
https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/tests/unit/test_image.py#L313-L325

[bendichter]

@h-mayorquin can you take a look at these conflicts? It would be great to get this PR through

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.73%. Comparing base (c0ba7e1) to head (206ee2a).

Additional details and impacted files

@@           Coverage Diff           @@
##              dev    #1538   +/-   ##
=======================================
  Coverage   91.73%   91.73%           
=======================================
  Files          27       27           
  Lines        2722     2722           
  Branches      710      709    -1     
=======================================
  Hits         2497     2497           
  Misses        149      149           
  Partials       76       76           

Flag	Coverage Δ
integration	73.03% <80.00%> (+0.07%)	⬆️
unit	82.29% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[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:

So I don't know how the Images, order of images, and friends would work very well here.

[h-mayorquin]

Timestamps does not make sense here because the images are repeated:

From the data, the rate is not very clear, I think it could be derived from the fluorescence timestamps but I don't really have the time to explore the dataset to that level.

[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 sort 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 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.