NeurodataWithoutBorders / nwbinspector

Tool to help inspect NWB files for compliance with NWB Best Practices
https://nwbinspector.readthedocs.io/
Other
17 stars 10 forks source link

[Bug]: Tests create a TimeSeries with a zero rate which is no longer allowed #421

Closed rly closed 2 months ago

rly commented 11 months ago

What happened?

This PR https://github.com/NeurodataWithoutBorders/pynwb/pull/1793 was recently merged into PyNWB. An error is now raised when a TimeSeries is created with a rate <= 0. The nwbinspector tests try to create such a TimeSeries and fail as a result.

https://github.com/NeurodataWithoutBorders/nwbinspector/blob/dev/tests/unit_tests/test_time_series.py#L208-L222

I think the first test should stay because old files may have a TimeSeries with a non-positive rate, but the test should be updated to create such a TimeSeries like so: https://github.com/NeurodataWithoutBorders/pynwb/blob/0e45cd927a0734428358eab10a75672e8dd75344/tests/unit/test_base.py#L414-L428

I do not understand the need for the second test above -- how often is a TimeSeries created with a single data point? That seems like an incorrect use of a TimeSeries and could be its own separate check, regardless of the rate specified.

Operating System

M1 macOS

Python Version

3.11

Were you streaming with ROS3?

None

Package Versions

No response

Code of Conduct

CodyCBakerPhD commented 11 months ago

I do not understand the need for the second test above -- how often is a TimeSeries created with a single data point?

Most often done for people with static images generated through microscopy techniques (I've seen it once or twice in last years) - we've even done it ourselves once or twice I believe - they could use the standard Image module types but then they can't associate all the lovely metadata about the device, imaging plane including grid spacing, optic channel, etc.

CodyCBakerPhD commented 11 months ago

@rly The NWB Inspector test is about equality at zero relative to the length of the first axis: https://github.com/NeurodataWithoutBorders/nwbinspector/blob/dev/src/nwbinspector/checks/time_series.py#L139

I commented on the original already merged PyNWB PR for this; as it stands this is a more fundamental misalignment between the two packages so we really should reach agreement. I think PyNWB should roll back that error to consider the length 1 time-axis condition, or otherwise make some official recommendation to how to better represent data in that case

Good to hear negative rates will no longer be allowed at the PyNWB level, but we don't actually seem to have a check for that yet (will raise an issue for that applied to old files) and note that tests for it will have to use your construct hack to get around it in order to imitate usage on older files

rly commented 11 months ago

Sounds good. Ah, I must have misread the code about checking for negative rates. Thanks for the detailed explanation here and in pynwb!