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

[Feature]: Warn on read rather than error #1786

Closed
3 tasks done
oruebel opened this issue Nov 12, 2023 · 1 comment · Fixed by #1793
Closed
3 tasks done

[Feature]: Warn on read rather than error #1786

oruebel opened this issue Nov 12, 2023 · 1 comment · Fixed by #1793
Assignees
Milestone

Comments

@oruebel
Copy link
Contributor

oruebel commented Nov 12, 2023

What would you like to see added to PyNWB?

In TimeSeries (and likely a few other places) additional error checks are performed on __init__ to ensure the data is valid. E.g,.:

pynwb/src/pynwb/base.py

Lines 175 to 191 in eb58506

if timestamps is not None:
if self.rate is not None:
raise ValueError('Specifying rate and timestamps is not supported.')
if self.starting_time is not None:
raise ValueError('Specifying starting_time and timestamps is not supported.')
self.fields['timestamps'] = timestamps
self.timestamps_unit = self.__time_unit
self.interval = 1
if isinstance(timestamps, TimeSeries):
timestamps.__add_link('timestamp_link', self)
elif self.rate is not None:
if self.starting_time is None: # override default if rate is provided but not starting time
self.starting_time = 0.0
self.starting_time_unit = self.__time_unit
else:
raise TypeError("either 'timestamps' or 'rate' must be specified")

This makes sense when creating a new file to ensure that we don't create invalid files. However, on read it would be useful to warn instead and continue to attempt to read the file to try and give access to the data if at all possible.

Is your feature request related to a problem?

Related to NeurodataWithoutBorders/helpdesk#62

What solution would you like?

Use the self._error_on_new_warn_on_construct method instead of ValueError in the TimeSeries constructor.

Do you have any interest in helping implement the feature?

Yes.

Code of Conduct

@oruebel oruebel added this to the Next Release milestone Nov 12, 2023
@rly
Copy link
Contributor

rly commented Nov 22, 2023

@stephprince stephprince self-assigned this Nov 28, 2023
stephprince added a commit that referenced this issue Nov 30, 2023
* change timeseries rate errors to warn on read (#1786, #1721)

* add test for starting time and update CHANGELOG

---------

Co-authored-by: Steph Prince <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants