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

add link spike_event_series to UnitSeries #825

Draft
wants to merge 4 commits into
base: enh/add_UnitSeries
Choose a base branch
from

Conversation

bendichter
Copy link
Contributor

Trying to get links working. Having some trouble. help @ajtritt?

@bendichter bendichter requested a review from ajtritt February 19, 2019 01:18
src/pynwb/ecephys.py Outdated Show resolved Hide resolved
@ajtritt
Copy link
Member

ajtritt commented Feb 19, 2019

the issues here should be fixed by 99281d7

@bendichter bendichter marked this pull request as ready for review February 19, 2019 22:00
@bendichter bendichter requested a review from oruebel February 19, 2019 22:01
@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #825 into enh/add_UnitSeries will decrease coverage by 0.05%.
The diff coverage is 63.15%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           enh/add_UnitSeries     #825      +/-   ##
======================================================
- Coverage               73.85%   73.79%   -0.06%     
======================================================
  Files                      60       60              
  Lines                    7191     7198       +7     
  Branches                 1514     1517       +3     
======================================================
+ Hits                     5311     5312       +1     
- Misses                   1453     1455       +2     
- Partials                  427      431       +4
Impacted Files Coverage Δ
src/pynwb/ecephys.py 97.36% <100%> (+0.02%) ⬆️
src/pynwb/form/build/map.py 72.2% <50%> (-0.19%) ⬇️
src/pynwb/form/spec/spec.py 78.7% <60%> (-0.23%) ⬇️
src/pynwb/form/utils.py 88.08% <0%> (-0.3%) ⬇️

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 2cfaae4...935c861. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #825 into enh/add_UnitSeries will decrease coverage by 0.05%.
The diff coverage is 63.15%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           enh/add_UnitSeries     #825      +/-   ##
======================================================
- Coverage               73.85%   73.79%   -0.06%     
======================================================
  Files                      60       60              
  Lines                    7191     7198       +7     
  Branches                 1514     1517       +3     
======================================================
+ Hits                     5311     5312       +1     
- Misses                   1453     1455       +2     
- Partials                  427      431       +4
Impacted Files Coverage Δ
src/pynwb/ecephys.py 97.36% <100%> (+0.02%) ⬆️
src/pynwb/form/build/map.py 72.2% <50%> (-0.19%) ⬇️
src/pynwb/form/spec/spec.py 78.7% <60%> (-0.23%) ⬇️
src/pynwb/form/utils.py 88.08% <0%> (-0.3%) ⬇️

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 2cfaae4...935c861. Read the comment docs.

@bendichter
Copy link
Contributor Author

Oliver wants to talk about this PR on Thursday before we merge

@bendichter
Copy link
Contributor Author

@oruebel what do you think? Can we merge this in?

@oruebel
Copy link
Contributor

oruebel commented Mar 12, 2019

Since this changes the schema, at the very least the version of the schema should then change as well. Since the change is backward compatible with 2.0.0, I guess the version should probably be 2.0.1. We then also need to update the release notes in the nwb-schema repo to document the schema change. I assume this change will also affect MatNWB, i.e., we would need to update it as well. We should create a guideline document for updating the schema, i.e., how changes get reviewed and approved and what steps we need to take to get them in.

@bendichter
Copy link
Contributor Author

bendichter commented Mar 12, 2019 via email

@bendichter
Copy link
Contributor Author

the same goes for #822

@ajtritt
Copy link
Member

ajtritt commented Jul 30, 2019

@bendichter @oruebel Has this PR gone the way of #822?

@ajtritt ajtritt mentioned this pull request Jul 30, 2019
5 tasks
@bendichter bendichter marked this pull request as draft June 24, 2020 22:49
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.

3 participants