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

Fix up scratch API #1309

Merged
merged 18 commits into from
Nov 4, 2020
Merged

Fix up scratch API #1309

merged 18 commits into from
Nov 4, 2020

Conversation

rly
Copy link
Contributor

@rly rly commented Oct 29, 2020

Motivation

Fix #1307, add missing unit tests, fix incorrect docval, clean up warnings and errors.

ScratchData requires notes, so set the default to empty string. A warning is raised in add_scratch if the notes argument is None or empty string. A warning is also raised in add_scratch if a pandas.DataFrame is provided and the table_description argument (required for DynamicTable) is None or empty string.

@bendichter I remember you saying something about empty strings being problematic. Could you elaborate?

Checklist

  • Have you checked our Contributing document?
  • Have you ensured the PR description clearly describes 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 #XXX notation where XXX is the issue number?

@oruebel
Copy link
Contributor

oruebel commented Oct 29, 2020

A key problem with empty string is that it is basically just side-stepping a required field by leaving it blank. I think requiring that users provide a description for the data they put into scratch is not much of a burden and at least provides some minimal documentation what those fields are about.

@rly
Copy link
Contributor Author

rly commented Nov 3, 2020

This PR is ready to go. Previously the default values for 'notes' and 'table_description' were empty string (both are required in the schema). I changed these arguments to be required, which is a breaking change. This will be pushed in the next version, PyNWB 2.0.0, which will include other breaking changes (e.g. TimeSeries.data will be required).

src/pynwb/file.py Outdated Show resolved Hide resolved
src/pynwb/file.py Outdated Show resolved Hide resolved
Copy link
Member

@ajtritt ajtritt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Oliver.

src/pynwb/file.py Outdated Show resolved Hide resolved
@rly rly requested review from oruebel and ajtritt November 3, 2020 21:49
ajtritt
ajtritt previously approved these changes Nov 3, 2020
src/pynwb/core.py Outdated Show resolved Hide resolved
oruebel
oruebel previously approved these changes Nov 3, 2020
@rly rly dismissed stale reviews from oruebel and ajtritt via 111c6b4 November 3, 2020 23:47
@rly rly merged commit 10820d8 into dev Nov 4, 2020
@rly rly deleted the enh/return_scratch branch November 4, 2020 00:10
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 this pull request may close these issues.

ScratchData notes passed as None instead of empty string in add_scratch
3 participants