Change timeseries data checks to warn instead of error on read

[stephprince]

Motivation

Fixes #1786 and #1721.

• Changes data validation checks for rate and timestamps to warnings if reading in data
• adds error for making a new time series with a non-positive rate. Will only give warning if reading in data with non-positive rate

How to test the behavior?

See example in #1721

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5e8ddc2) 91.96% compared to head (e8bd8f8) 91.97%.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev    #1793   +/-   ##
=======================================
  Coverage   91.96%   91.97%           
=======================================
  Files          27       27           
  Lines        2615     2617    +2     
  Branches      682      683    +1     
=======================================
+ Hits         2405     2407    +2     
  Misses        138      138           
  Partials       72       72           

Flag	Coverage Δ
integration	71.22% <0.00%> (-0.06%)	⬇️
unit	83.64% <100.00%> (+0.01%)	⬆️

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.

[rly]

Please add a new entry or two to CHANGELOG.md under "PyNWB 2.6.0 (Upcoming)" > "Enhancements and minor changes" with a brief description of the changes.

[rly]

The nwbinspector tests fail because they include tests that create a non-positive rate. With this PR, those tests will raise an error. Pinging @CodyCBakerPhD - these nwbinspector tests will need to be updated after this is released in PyNWB 2.6.0.

[CodyCBakerPhD]

@rly @stephprince Hey there

Does this mean that on next release of PyNWB it will be impossible to specify any TimeSeries with a rate of zero, even if it only has a single element on the first axis?

That's a fairly big break to back-compatibility if so, we've recommended to several people throughout the years to use that approach for statically generated microscopy images since that is the closest available data type for their setup (static images module lacks the associated metadata, and no specific static image types in the ophys module)

Not that it's a bad thing overall, but there should be a bigger changelog note about it IMO, along with some update about what we recommend to such users as an alternative

[oruebel]

Thanks, I was not aware that folks were using the Ophys modules this way. Could you clarify why rate matters when there is only a single image. Wouldn't you just use an explicit timestamp array in that case? I think it is fine to change the check to only check for "self.rate < 0" instead of "self.rate <= 0" to avoid the breaking case you mentioned (an maybe have an warning for "rate == 0").

[CodyCBakerPhD]

I guess the difference with that proposal is then how to treat NaN values for timestamps

For static images in this case, it might not matter when it was acquired w.r.t. any other data streams in the file, so the starting_time could be set to NaN while the rate is set to a true value of 0.0

It's true an equivalent representation could be to use timestamps but then it would be an array of one element, that being NaN, but we'd need checks elsewhere to ensure no more than one NaN is set in other instances, such as a series with timestamps=[NaN, NaN, NaN, ...] which would make less sense, right?

In all cases, this is really because there's a lack of appropriate neurodata object, so maybe I'll just kick the bucket to ndx-microscopy to resolve this in the best way

I do think we shouldn't break back-compatibility for the exact 0.0 case for the time being however, just so there's no unintended disruption

[oruebel]

In this case I would suggest the following:

• change the new check to only check for self.rate > 0
• optionally add a warning for self.rate == 0
• Wait for ndx-microscopy to address the missing data type

[oruebel]

@stephprince could you create a PR to make those changes

[stephprince]

yes I can do that