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 warnings, use keyword args where possible #1484

Merged
merged 7 commits into from
May 26, 2022
Merged

Fix warnings, use keyword args where possible #1484

merged 7 commits into from
May 26, 2022

Conversation

rly
Copy link
Contributor

@rly rly commented May 25, 2022

Use the just released HDMF 3.3.1. Note that HDMF 3.3 introduces warnings when positional arguments are used in subclasses of many base types. This PR replaces positional args with keyword args in source code and tests throughout. Fix #1189

Also remove old-style super() calls and calls to call_docval_func

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the 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 "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #1484 (1bde866) into dev (728e0ac) will increase coverage by 0.03%.
The diff coverage is 95.43%.

@@            Coverage Diff             @@
##              dev    #1484      +/-   ##
==========================================
+ Coverage   78.24%   78.27%   +0.03%     
==========================================
  Files          37       37              
  Lines        2799     2757      -42     
  Branches      473      486      +13     
==========================================
- Hits         2190     2158      -32     
+ Misses        524      518       -6     
+ Partials       85       81       -4     
Impacted Files Coverage Δ
src/pynwb/io/ophys.py 45.45% <0.00%> (ø)
src/pynwb/io/base.py 58.62% <50.00%> (ø)
src/pynwb/__init__.py 76.64% <80.00%> (ø)
src/pynwb/spec.py 89.07% <83.33%> (ø)
src/pynwb/ophys.py 94.20% <91.30%> (-1.14%) ⬇️
src/pynwb/file.py 87.56% <93.93%> (-0.07%) ⬇️
src/pynwb/base.py 100.00% <100.00%> (+4.76%) ⬆️
src/pynwb/behavior.py 100.00% <100.00%> (ø)
src/pynwb/core.py 65.85% <100.00%> (ø)
src/pynwb/device.py 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 728e0ac...1bde866. Read the comment docs.

@rly
Copy link
Contributor Author

rly commented May 26, 2022

This looks like a really big PR, but no functionality has changed.

This PR does not introduce warnings for using positional arguments for most NWB neurodata type classes, but sets the foundation for it. That change will be in the next minor release.

A minor bug was fixed in src/pynwb/io/epoch.py

There is still a warning about the length of data not matching the length of timestamps when an ImageSeries is created where an external file is used and timestamps are provided. In this case, a placeholder 'data' is set with shape (0,0,0). Again, this is a case where ImageSeries, when not used to store data in the data field breaks inheritance rules and causes headache. I think we should revisit creation of a new neurodata type that is just for external file backed ImageSeries. See also NeurodataWithoutBorders/nwb-schema#481 and #1220 and #1274

@rly rly requested review from bendichter and oruebel May 26, 2022 07:04
@rly rly merged commit d38cc78 into dev May 26, 2022
@rly rly deleted the fix_warnings branch May 26, 2022 16:44
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.

Use keyword-only arguments in neurodata_type constructors
2 participants