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 classification of attributes as new/overridden and issue with custom spec keys #503

Merged
merged 14 commits into from
Apr 22, 2021

Conversation

rly
Copy link
Contributor

@rly rly commented Dec 23, 2020

Motivation

Fix #501, address part of #502, and fix another related bug where cached HDMF schema loaded from a NWB file were not being resolved correctly.

When extracting HDMF out of PyNWB, a few keys were renamed to be more general, e.g., 'neurodata_type_def' -> 'data_type_def', and subclasses of GroupSpec, DatasetSpec, and SpecNamespace in HDMF were created in PyNWB. These NWB-specific subclasses are used when reading the hdmf-common schema cached within a file, but the cached schema uses 'data_type_def' instead of 'neurodata_type_def'. This prevented hdmf-common specs from being resolved correctly so that attributes, datasets, groups, and links defined by a base type within hdmf-common were in some cases not inherited by a type that extends the base type.

I also refactored parts of spec.py, added comments, and added additional tests to push code coverage of spec.py from 79% to 83% and namespace.py from 79% to 90%.

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.

@rly rly changed the title Fix classification of attributes as new/overridden Fix classification of attributes as new/overridden and issue with custom spec keys Dec 23, 2020
@codecov
Copy link

codecov bot commented Dec 23, 2020

Codecov Report

Merging #503 (ce6509f) into dev (c92378a) will decrease coverage by 0.99%.
The diff coverage is 92.00%.

❗ Current head ce6509f differs from pull request most recent head 1a1ad99. Consider uploading reports for the commit 1a1ad99 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #503      +/-   ##
==========================================
- Coverage   85.47%   84.48%   -1.00%     
==========================================
  Files          41       38       -3     
  Lines        8213     7700     -513     
  Branches     1770     1657     -113     
==========================================
- Hits         7020     6505     -515     
- Misses        842      861      +19     
+ Partials      351      334      -17     
Impacted Files Coverage Δ
src/hdmf/backends/hdf5/h5tools.py 79.51% <ø> (-1.99%) ⬇️
src/hdmf/spec/namespace.py 87.50% <90.32%> (+3.20%) ⬆️
src/hdmf/spec/spec.py 83.39% <94.73%> (+2.40%) ⬆️
src/hdmf/common/io/table.py 60.00% <0.00%> (-23.51%) ⬇️
src/hdmf/validate/validator.py 75.00% <0.00%> (-5.48%) ⬇️
src/hdmf/validate/errors.py 78.94% <0.00%> (-5.37%) ⬇️
src/hdmf/spec/write.py 81.20% <0.00%> (-4.70%) ⬇️
src/hdmf/common/__init__.py 71.02% <0.00%> (-3.14%) ⬇️
src/hdmf/build/manager.py 79.53% <0.00%> (-2.58%) ⬇️
... and 15 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 c92378a...1a1ad99. Read the comment docs.

@rly rly requested a review from a team April 22, 2021 09:05
@rly
Copy link
Contributor Author

rly commented Apr 22, 2021

This should resolve the error in PyNWB mentioned here: #550 (comment)


def is_many(self):
return self.quantity not in (1, ZERO_OR_ONE)

@classmethod
def get_data_type_spec(cls, data_type_def):
def get_data_type_spec(cls, data_type_def): # unused
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume 'unused' means they are never called anywhere. Are you leaving these here just in case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a note-to-self. Reading the code, one might think it would be used in every data_type spec, but actually it is not used anywhere.

@rly rly merged commit e7888b5 into dev Apr 22, 2021
@rly rly deleted the fix/spec_attrs branch April 22, 2021 18:19
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.

ValueError: Field 'description' cannot be defined in FRETSeries. It already exists on base class ImageSeries.
3 participants