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 common data structures #134

Merged
merged 42 commits into from
Sep 26, 2019
Merged

Add common data structures #134

merged 42 commits into from
Sep 26, 2019

Conversation

ajtritt
Copy link
Contributor

@ajtritt ajtritt commented Aug 22, 2019

Motivation

HDMF is used to build PyNWB. There are some useful data structures in PyNWB that have been moved into HDMF-common for use in other HDMF applications. The class/types that have been moved are DynamicTable and its dependencies. Additionally, a data type for storing compressed row sparse matrices.

These types will be introduced into the HDMF-common schema by hdmf-dev/hdmf-common-schema#1

Fix #60. This also moves some useful functionality from NWBContainer over to the general Container class.

Notable changes:

  • To move NWBContainer over to Container, a new AbstractContainer class was created, and many isinstance(..., Container) were changed to isinstance(..., AbstractContainer).
  • the data constructor argument for Data objects (i.e. parent of NWBData) is now automatically populated by the ObjectMapper (See here)

@ajtritt ajtritt requested review from rly, bendichter and oruebel August 22, 2019 23:10
@bendichter
Copy link
Contributor

Will this satisfy #60?

oruebel
oruebel previously approved these changes Aug 22, 2019
rly
rly previously approved these changes Aug 22, 2019
@ajtritt ajtritt dismissed stale reviews from rly and oruebel via 847a3c0 September 7, 2019 00:04
@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #134 into dev will increase coverage by 0.53%.
The diff coverage is 58.15%.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev     #134      +/-   ##
=========================================
+ Coverage   69.2%   69.73%   +0.53%     
=========================================
  Files         24       29       +5     
  Lines       4981     5726     +745     
  Branches    1143     1351     +208     
=========================================
+ Hits        3447     3993     +546     
- Misses      1168     1306     +138     
- Partials     366      427      +61
Impacted Files Coverage Δ
src/hdmf/spec/spec.py 68.24% <ø> (+1.43%) ⬆️
src/hdmf/__init__.py 50% <100%> (ø) ⬆️
src/hdmf/backends/hdf5/h5_utils.py 59.53% <100%> (+0.47%) ⬆️
src/hdmf/common/io/__init__.py 100% <100%> (ø)
src/hdmf/spec/namespace.py 74.53% <30%> (+2.5%) ⬆️
src/hdmf/data_utils.py 83.72% <50%> (-0.23%) ⬇️
src/hdmf/common/table.py 53.61% <53.61%> (ø)
src/hdmf/container.py 66.27% <56.31%> (-27.55%) ⬇️
src/hdmf/common/io/table.py 58.82% <58.82%> (ø)
src/hdmf/build/map.py 66.12% <65.11%> (+5.26%) ⬆️
... 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 62d487e...462f32d. Read the comment docs.

for subspec, value in subspecs.items():
# FOR SOME REASON, no specs are getting mapped to the 'data' constructor argument
Copy link
Contributor

Choose a reason for hiding this comment

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

can this comment be removed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

more remnants of debugging

ret = ret['namespaces']
if self.__cache is None:
self.__cache = self.__read(ns_path)
ret = self.__cache['namespaces']
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: caching of the spec by the H5SpecReader should be mentioned in the release notes

@@ -152,6 +152,7 @@ class ValidatorMap(object):
def __init__(self, **kwargs):
ns = getargs('namespace', kwargs)
self.__ns = ns
self._ns = ns
Copy link
Contributor

Choose a reason for hiding this comment

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

What was this addition for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remnants of debugging... I'll remove that

{'name': 'colnames', 'type': 'array_data', 'doc': 'the names of the columns in this table',
'default': None})
def __init__(self, **kwargs):
id, columns, desc, colnames = popargs('id', 'columns', 'description', 'colnames', kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

The init for DynamicTable is quite complex. It would be nice to add some documentation to indicate what happens where.


@staticmethod
def __build_columns(columns, df=None):
tmp = list()
Copy link
Contributor

Choose a reason for hiding this comment

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

Add docstring to describe what __build_columns does


@classmethod
def _getter(cls, field):
doc = field.get('doc')
Copy link
Contributor

Choose a reason for hiding this comment

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

Add docstring for _getter


@staticmethod
def _transform_arg(field):
tmp = field
Copy link
Contributor

Choose a reason for hiding this comment

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

Add docstring for _transform_arg

oruebel
oruebel previously approved these changes Sep 26, 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.

This looks good to me. Great job in porting all this functionality over. I added few comments to fix some documentation issues. In addition, it would be useful to also updated the developer docs to mention the submodules in this repo and also describe how to checkout and updated the submodules

@ajtritt ajtritt merged commit 7705c59 into dev Sep 26, 2019
@oruebel
Copy link
Contributor

oruebel commented Sep 26, 2019

@ajtritt @rly Thanks for the hard push on this.

@rly rly deleted the enh/common branch September 26, 2019 20:33
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.

Move NWBBaseType functionality to Container
4 participants