Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2156 +/- ##
==========================================
+ Coverage 95.23% 95.31% +0.07%
==========================================
Files 29 30 +1
Lines 2876 2923 +47
Branches 740 751 +11
==========================================
+ Hits 2739 2786 +47
Misses 83 83
Partials 54 54
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
|
After discussion with @oruebel and @bendichter , we decided to not deprecate the "instantaneous" value for |
Updated the docstring to clarify that timestamps are in seconds from the session start time and removed the default description.
| # :py:class:`~pynwb.behavior.BehavioralEvents` is deprecated. Use | ||
| # :py:class:`~pynwb.event.EventsTable` instead. | ||
| # | ||
| # TODO: Add EventsTable example |
There was a problem hiding this comment.
I assume this TODO item should be resolved here?
| # For storing events with annotations (e.g., stimulus events), use :py:class:`~pynwb.event.EventsTable` | ||
| # in ``NWBFile.events``. | ||
| # | ||
| # TODO: Add EventsTable example |
There was a problem hiding this comment.
Same as above. Open TODO item in the docs.
| self.add_timeseries(time_series) | ||
| self._error_on_new_warn_on_construct( | ||
| "BehavioralEvents is deprecated. Use an EventsTable in NWBFile.events instead for event data. " | ||
| "Creating a new BehavioralEvents will not be allowed in a future version of PyNWB." |
There was a problem hiding this comment.
The message says will not be allowed in a future version of PyNWB. but the implementation already raises an error an. I.e., either the message should be updated or this should be a DeprecationWarning instead. Also, do we usually raise warnings on read for deprecated types?
There was a problem hiding this comment.
Thanks. I'm not sure what I was thinking. This should be warn on new, pass on construct. I'll fix it.
There was a problem hiding this comment.
There are a handful of cases where a setting shouldn't have been allowed before, where we do error on new, warn on construct
| if self.continuity == "instantaneous": | ||
| self._error_on_new_warn_on_construct( | ||
| "The 'instantaneous' value for TimeSeries.continuity is deprecated. " | ||
| "Use an EventsTable in NWBFile.events instead for event data. " | ||
| "This value will not be allowed to be set in a future version of PyNWB." | ||
| ) | ||
|
|
There was a problem hiding this comment.
Just to flag this. This should be removed per the team discussion earlier
| self._error_on_new_warn_on_construct( | ||
| "AnnotationSeries is deprecated. Use an EventsTable with an 'annotation' column instead. " | ||
| "Creating a new AnnotationSeries will not be allowed in a future version of PyNWB." | ||
| ) |
There was a problem hiding this comment.
Same question as above. Is the intent to disallow or to deprecate? I.e., we should either update the error message or use a deprecation warning instead.
3 participants
Motivation
How to test the behavior?
Checklist
ruff check . && codespellfrom the source directory.