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

Remove unused _ancestry class variable #707

Merged

Conversation

t-b
Copy link
Collaborator

@t-b t-b commented Nov 3, 2018

This is not written to NWB anymore.

This is not written to NWB anymore.
@t-b t-b requested review from bendichter and ajtritt November 3, 2018 13:54
@codecov
Copy link

codecov bot commented Nov 3, 2018

Codecov Report

Merging #707 into dev will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #707      +/-   ##
==========================================
- Coverage   74.52%   74.42%   -0.11%     
==========================================
  Files          59       59              
  Lines        6849     6830      -19     
  Branches     1415     1415              
==========================================
- Hits         5104     5083      -21     
- Misses       1354     1355       +1     
- Partials      391      392       +1
Impacted Files Coverage Δ
src/pynwb/behavior.py 100% <ø> (ø) ⬆️
src/pynwb/image.py 100% <ø> (ø) ⬆️
src/pynwb/ophys.py 94.54% <ø> (-0.1%) ⬇️
src/pynwb/ecephys.py 97.27% <ø> (-0.05%) ⬇️
src/pynwb/misc.py 98.43% <ø> (-0.07%) ⬇️
src/pynwb/icephys.py 88.88% <ø> (-0.59%) ⬇️
src/pynwb/ogen.py 100% <ø> (ø) ⬆️
src/pynwb/form/utils.py 86.76% <0%> (-0.59%) ⬇️

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 bfb5c83...e382672. Read the comment docs.

@bendichter
Copy link
Contributor

Wait I actually like this. Is there currently no way to know from the nwb file how a class was derived? Would analysis tools benefit from access to this variable? Eg knowing that an extension derived from TimeSeries? If so it would probably be easier to do this automatically.

@oruebel
Copy link
Contributor

oruebel commented Nov 3, 2018

ancestry should certainly not be stored in the file. That is the role of the schema and having the ancestry separately in the file is problematic.

@bendichter
Copy link
Contributor

I guess the cached schema would accomplish this

@bendichter
Copy link
Contributor

I don't really understand what's "problematic" about this though

@t-b
Copy link
Collaborator Author

t-b commented Nov 3, 2018

From the cached spec in the file the ancestry can always be recovered.

@ajtritt
Copy link
Member

ajtritt commented Nov 5, 2018

@bendichter If you want ancestry, we could have the register_class decorator add it as a class attribute (this is how the neurodata_type class attribute gets added). Hardcoding it as a comma delimited string is problematic for long term maintenance and consistency. It’s just a field that would need to be kept in sync with the schema

@bendichter
Copy link
Contributor

@ajtritt thanks for the explanation. @oruebel also pointed me to the discussion in the change logs. Let's remove this inactive code. Instead, how would you all feel about implementing a function that is analogous to isinstance but for neurodata_types instead of python classes using the schema?

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.

looks good to me. Thanks @t-b

@ajtritt
Copy link
Member

ajtritt commented Nov 7, 2018

Instead, how would you all feel about implementing a function that is analogous to isinstance but for neurodata_types instead of python classes using the schema?

@bendichter that seems reasonable. I prefer we address that under a new issue though

@t-b
Copy link
Collaborator Author

t-b commented Nov 8, 2018

@ajtritt @bendichter "Merge pull request" <-- Click here

@bendichter bendichter merged commit 9f4105c into NeurodataWithoutBorders:dev Nov 8, 2018
@t-b t-b deleted the bugfix/remove-unused-ancestry branch November 8, 2018 16:34
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