Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 tasks done
rly opened this issue Dec 13, 2023 · 3 comments
Closed
3 tasks done
Labels
category: bug errors in the code or code behavior

Comments

@rly
Copy link
Contributor

rly commented Dec 13, 2023

What happened?

This PR NeurodataWithoutBorders/pynwb#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

  • I agree to follow this project's Code of Conduct
  • Have you ensured this bug was not already reported?
  • To the best of your ability, have you ensured this is a bug within the code that checks the NWBFile, rather than a bug in the NWBFile reader (e.g., PyNWB or MatNWB)?
@rly rly added the category: bug errors in the code or code behavior label Dec 13, 2023
@CodyCBakerPhD
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

rly commented Dec 18, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior
Projects
None yet
Development

No branches or pull requests

2 participants