Conversation
lepmik
commented
May 16, 2019
lepmik
commented
- References and RegionReferences
- Stored in datasets
Codecov Report
@@ Coverage Diff @@
## dev #90 +/- ##
==========================================
+ Coverage 95.57% 96.02% +0.44%
==========================================
Files 11 14 +3
Lines 1604 1935 +331
==========================================
+ Hits 1533 1858 +325
- Misses 71 77 +6
Continue to review full report at Codecov.
|
dragly
left a comment
dragly
left a comment
There was a problem hiding this comment.
I had completely forgotten about this PR until I saw your latest commit @lepmik.
I glanced quickly over it now and spotted a few immediate issues that should be addressed. I am not sure if you are still planning to merge this? I figured I should ask before I continue the review.
| def special_dtype(**kwds): | ||
| """ Create a new h5py "special" type. Only one keyword may be given. | ||
| Legal keywords are: | ||
| vlen = basetype | ||
| Base type for HDF5 variable-length datatype. This can be Python | ||
| str type or instance of np.dtype. | ||
| Example: special_dtype( vlen=str ) | ||
| enum = (basetype, values_dict) | ||
| Create a NumPy representation of an HDF5 enumerated type. Provide | ||
| a 2-tuple containing an (integer) base dtype and a dict mapping | ||
| string names to integer values. | ||
| ref = Reference | RegionReference | ||
| Create a NumPy representation of an HDF5 object or region reference | ||
| type. | ||
| """ |
There was a problem hiding this comment.
This code is copied from h5py. This file should therefore have a header stating that it originates from h5py and has a BSD license. Something similar to their own headers is fine, except it should not state that it is part of h5py, of course:
# This file is part of h5py, a Python interface to the HDF5 library.
#
# http://www.h5py.org
#
# Copyright 2008-2019 Andrew Collette and contributors
#
# License: Standard 3-clause BSD; see "license.txt" for full license terms
# and contributor agreement.
In addition, we should add a third-party-licenses/h5py.txt file to the repo.
The mentions of HDF5 and h5py in the code itself should probably be replaced with exdir to avoid confusion.
| def check_string_dtype(dt): | ||
| """If the dtype represents an HDF5 string, returns a string_info object. | ||
| The returned string_info object holds the encoding and the length. | ||
| The encoding can only be 'utf-8' or 'ascii'. The length may be None | ||
| for a variable-length string, or a fixed length in bytes. | ||
| Returns None if the dtype does not represent an HDF5 string. | ||
| """ | ||
| vlen_kind = check_vlen_dtype(dt) | ||
| if vlen_kind is unicode: | ||
| return string_info('utf-8', None) | ||
| elif vlen_kind is bytes: | ||
| return string_info('ascii', None) | ||
| elif dt.kind == 'S': | ||
| enc = (dt.metadata or {}).get('h5py_encoding', 'ascii') | ||
| return string_info(enc, dt.itemsize) | ||
| else: | ||
| return None |
There was a problem hiding this comment.
I think this is unused. We should probably remove unused functions unless they are supposed to be part of the public API.
| # TODO add the code below for testing true equality when parallelizing | ||
| # def __eq__(self, other): | ||
| # self[:] | ||
| # if isinstance(other, self.__class__): | ||
| # other[:] | ||
| # if self.__dict__.keys() != other.__dict__.keys(): | ||
| # return False | ||
| # | ||
| # for key in self.__dict__: | ||
| # if key == "_data": | ||
| # if not np.array_equal(self.__dict__["_data"], other.__dict__["_data"]): | ||
| # return False | ||
| # else: | ||
| # if self.__dict__[key] != other.__dict__[key]: | ||
| # return False | ||
| # return True | ||
| # else: | ||
| # return False |
There was a problem hiding this comment.
We should probably remove TODOs before merging.
|
Not planning on working with this now, it was originally started upon to enable integration of exdir into NWB, so we can probably freeze this for the time being. |
2 participants
