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 type hierarchy for CorrectedImageStack, ImagingRetinotopy; improve docs #962

Merged
merged 13 commits into from
Jun 11, 2019

Conversation

rly
Copy link
Contributor

@rly rly commented May 28, 2019

Fixes NeurodataWithoutBorders/nwb-schema#271 and improve documentation

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 ?

@codecov
Copy link

codecov bot commented May 28, 2019

Codecov Report

Merging #962 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #962   +/-   ##
=======================================
  Coverage   72.93%   72.93%           
=======================================
  Files          36       36           
  Lines        2609     2609           
  Branches      497      497           
=======================================
  Hits         1903     1903           
  Misses        590      590           
  Partials      116      116
Impacted Files Coverage Δ
src/pynwb/ecephys.py 97.29% <100%> (ø) ⬆️

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 e9bf8d5...affb676. Read the comment docs.

@codecov
Copy link

codecov bot commented May 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (dev@565ba8a). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev     #962   +/-   ##
======================================
  Coverage       ?   73.24%           
======================================
  Files          ?       36           
  Lines          ?     2616           
  Branches       ?      496           
======================================
  Hits           ?     1916           
  Misses         ?      584           
  Partials       ?      116
Impacted Files Coverage Δ
src/pynwb/ecephys.py 97.29% <100%> (ø)

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 565ba8a...42b8c54. Read the comment docs.

Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

You should also update the patch version of the namespace, i.e., 2.0.x to 2.0.(x+1) since your are changing the schema.

In NeurodataWithoutBorders/nwb-schema#271 you should then also update the release of the nwb-schema docs.

Luckily, NWBDataInterface does not any fields so this change should not affect compatability.

@rly
Copy link
Contributor Author

rly commented May 29, 2019

Regarding updated author list, see also updates to nwb-schema: NeurodataWithoutBorders/nwb-schema#265 and NeurodataWithoutBorders/nwb-schema#266

oruebel
oruebel previously approved these changes May 29, 2019
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

I forgot #953 also changes the version, i.e., depending on the order these get merged we may need to update the version again.

Also t-b discovered in #953 another issue with the version in that we need to update the value of the nwb_version attribute in nwb.file.yaml as well. While this is a separate issue that we should fix, we should in the meantime make sure to update both until we have fixed the problem with having nwb_version as attribute in the schema.

@rly
Copy link
Contributor Author

rly commented May 29, 2019

Can we put all of these schema changes under 2.0.2 or should they be kept separate?

@rly
Copy link
Contributor Author

rly commented May 29, 2019

#822 and #825 seem ready to merge with an update to the schema as well.

@oruebel
Copy link
Contributor

oruebel commented May 29, 2019

Can we put all of these schema changes under 2.0.2 or should they be kept separate?

Putting #953 and this PR under the 2.0.2 umbrella seems fine. #822 and #825 add new functionality and should be part of a separate version jump.

#822 and #825 seem ready to merge with an update to the schema as well.

I remember we had some discussions about those PRs, which is why we did not merge those yet, but I don't recall what the issue was. @ajtritt do you recall if #822 and #825 were good to merge now?

@rly rly merged commit 2059c07 into dev Jun 11, 2019
@rly rly deleted the minor_fixes branch June 11, 2019 21:01
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.

Make CorrectedImageStack and ImagingRetinotopy inherit from NWBDataInterface
4 participants