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

Enh/device description #723

Closed
wants to merge 10 commits into from
Closed

Enh/device description #723

wants to merge 10 commits into from

Conversation

ajtritt
Copy link
Member

@ajtritt ajtritt commented Nov 12, 2018

Motivation

After #695, Device became a group with a help string. This PR adds a required description field. A Device ObjectMapper was also included for setting a default value for those files generated prior to this change.

This change was inspired by discussions with @ageorgou

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
oruebel previously approved these changes Nov 12, 2018
@oruebel
Copy link
Contributor

oruebel commented Nov 12, 2018

Looks like some of tests need to be updated, but other than that it looks good to me.

@bendichter
Copy link
Contributor

bendichter commented Nov 12, 2018

I prefer these types of additions to be optional. This change will require changes to dozen different conversion scripts and does not offer me much utility. I don't think these types of descriptive fields need to be required

@ajtritt
Copy link
Member Author

ajtritt commented Nov 12, 2018

Then we should just remove Device. A group with a name and a constant help string doesn’t do much.

@codecov
Copy link

codecov bot commented Nov 12, 2018

Codecov Report

Merging #723 into dev will increase coverage by 2.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev     #723     +/-   ##
=========================================
+ Coverage   72.29%   74.49%   +2.2%     
=========================================
  Files          36       60     +24     
  Lines        2718     6905   +4187     
  Branches      513     1444    +931     
=========================================
+ Hits         1965     5144   +3179     
- Misses        632     1355    +723     
- Partials      121      406    +285
Impacted Files Coverage Δ
src/pynwb/device.py 100% <100%> (ø) ⬆️
src/pynwb/epoch.py 73.13% <0%> (-9.22%) ⬇️
src/pynwb/__init__.py 69.49% <0%> (-2.55%) ⬇️
src/pynwb/core.py 70.51% <0%> (-0.87%) ⬇️
src/pynwb/icephys.py 88.88% <0%> (-0.4%) ⬇️
src/pynwb/ecephys.py 97.27% <0%> (-0.1%) ⬇️
src/pynwb/ogen.py 100% <0%> (ø) ⬆️
src/pynwb/behavior.py 100% <0%> (ø) ⬆️
src/pynwb/validate.py 0% <0%> (ø) ⬆️
src/pynwb/retinotopy.py 100% <0%> (ø) ⬆️
... and 32 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 2cd7713...0092214. Read the comment docs.

Copy link
Contributor

@bendichter bendichter left a comment

Choose a reason for hiding this comment

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

making Device optional obviates the Device io file

@bendichter
Copy link
Contributor

The addition of description to the tests is now superfluous, but I don't mind leaving them

@@ -235,6 +235,10 @@ groups:
dtype: text
doc: Value is 'A recording device e.g. amplifier'.
value: A recording device e.g. amplifier
datasets:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a dataset and not an attribute? I would not expect this to be changed once it is written, so shouldn't it be an attribute (NeurodataWithoutBorders/nwb-schema#48)? Making this required would be a schema-breaking change, so I would like to make this optional.

@rly rly closed this Jun 24, 2020
@rly rly deleted the enh/device_description branch June 24, 2020 22:46
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.

4 participants