Repository: glotzerlab/signac
Pull request #115 (2019-02-05 20:26:02)

Feature/h5store support storing of hdf5 data sets and mappings
Explicitly support `h5py` datasets and mappings as valid types for the `H5Store` class.

## Description

This patch special-case handles both h5py data sets and mappings during assignments and equality checks. An exception is raised when a user attempts to set an existing group to the same key.

## Motivation and Context

Changes introduced here are aimed at increasing the versatility and robustness of the `H5Store`.
The `H5Store` should look-and-feel as much as *reasonable* like a `dict` without compromising the inherent advantages of the underlying file format.

## Types of Changes
<!-- Please select all items that apply either now or after creating the pull request: -->
- [ ] Documentation update
- [ ] Bug fix
- [x] New feature
- [x] Breaking change<sup>1</sup>

This pull request is based on
- [ ] *master*, because it is a bug fix or an update to the documentation.
- [x] *develop*, because it introduces a new feature.

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
<!-- Please select all items that apply either now or after creating the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [x] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.txt).
- [x] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:
- [ ] I have updated the API documentation as part of the package doc-strings.
- [ ] I have created a separate pull request to update the [framework documentation](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt).


csadorf: I'm going to address the failing unit tests now.
csadorf: Ok, this appears to be ready for review now.

Patch:
diff --git a/signac/core/h5store.py b/signac/core/h5store.py
index d7a365a1..ba51b329 100644
--- a/signac/core/h5store.py
+++ b/signac/core/h5store.py
@@ -55,12 +55,25 @@ def _requires_tables():
 logger = logging.getLogger(__name__)
 
 
+class ClosedH5StoreError(RuntimeError):
+    "Raised when trying to access a closed group."
+    pass
+
+
 def _h5set(file, grp, key, value, path=None):
     """Set a key in an h5py container, recursively converting Mappings to h5py
     groups and transparently handling None."""
+    import h5py
     import numpy    # h5py depends on numpy, so this is safe.
     path = path + '/' + key if path else key
 
+    # Guard against assigning a group to itself, e.g., `h5s[key] = h5s[key]`,
+    # where h5s[key] is a mapping. This is necessary, because the original
+    # mapping would be deleted prior to assignment.
+    if isinstance(value, H5Group) and key in grp:
+        if grp[key] == value._group:
+            return  # Groups are identical, do nothing.
+
     # Delete any existing data
     if key in grp:
         del grp[key]
@@ -83,6 +96,10 @@ def _h5set(file, grp, key, value, path=None):
     elif type(value).__module__ == numpy.__name__:
         grp[key] = value
 
+    # h5py native types
+    elif isinstance(value, h5py._hl.dataset.Dataset):
+        grp[key] = value[()]    # Create a copy, not a hard link!
+
     # Other types
     else:
         _load_pandas()   # might be a pandas type
@@ -167,7 +184,7 @@ class H5Group(MutableMapping):
 
     @property
     def _group(self):
-        return self._file._file[self._path]
+        return self._file.file[self._path]
 
     def __getitem__(self, key):
         with _ensure_open(self._file):
@@ -208,7 +225,9 @@ class H5Group(MutableMapping):
 
     def __eq__(self, other):
         with _ensure_open(self._file):
-            if type(other) == type(self):
+            if isinstance(self, Mapping) and isinstance(other, Mapping):
+                return super(H5Group, self).__eq__(other)
+            elif type(other) == type(self):
                 return self._group == other._group
             else:
                 return super(H5Group, self).__eq__(other)
@@ -278,6 +297,13 @@ class H5Store(MutableMapping):
             if locked:
                 self._thread_lock.release()
 
+    @property
+    def file(self):
+        if self._file is None:
+            raise ClosedH5StoreError(self._filename)
+        else:
+            return self._file
+
     def flush(self):
         self._file.flush()
 


Test patch:
diff --git a/tests/test_h5store.py b/tests/test_h5store.py
index 0c8f479c..622e8991 100644
--- a/tests/test_h5store.py
+++ b/tests/test_h5store.py
@@ -15,7 +15,7 @@ from platform import python_implementation
 from multiprocessing.pool import ThreadPool
 from contextlib import closing
 
-from signac.core.h5store import H5Store
+from signac.core.h5store import H5Store, ClosedH5StoreError
 from signac.common import six
 from signac.warnings import SignacDeprecationWarning
 
@@ -54,6 +54,7 @@ class BaseH5StoreTest(unittest.TestCase):
     def setUp(self):
         self._tmp_dir = TemporaryDirectory(prefix='signac_test_h5store_')
         self._fn_store = os.path.join(self._tmp_dir.name, FN_STORE)
+        self._fn_store_other = os.path.join(self._tmp_dir.name, 'other_' + FN_STORE)
         self.addCleanup(self._tmp_dir.cleanup)
 
     def get_h5store(self):
@@ -64,6 +65,14 @@ class BaseH5StoreTest(unittest.TestCase):
         with self.get_h5store() as h5s:
             yield h5s
 
+    def get_other_h5store(self):
+        return H5Store(filename=self._fn_store_other)
+
+    @contextmanager
+    def open_other_h5store(self):
+        with self.get_other_h5store() as h5s:
+            yield h5s
+
     def get_testdata(self, size=None):
         if size is None:
             size = 1024
@@ -89,6 +98,8 @@ class H5StoreTest(BaseH5StoreTest):
         'double_array': array('d', [-1.5, 0, 1.5]),
         'int_array': array('i', [-1, 0, 1]),
         'uint_array': array('I', [0, 1, 2]),
+        'numpy_float_array': numpy.array([-1.5, 0, 1.5], dtype=float),
+        'numpy_int_array': numpy.array([-1, 0, 1], dtype=int),
         'dict': {
             'a': 1,
             'b': None,
@@ -249,6 +260,45 @@ class H5StoreTest(BaseH5StoreTest):
                 h5s[k] = v
                 self.assertEqual(h5s[k], v)
 
+    def test_assign_valid_types_within_identical_file(self):
+        with self.open_h5store() as h5s:
+            for k, v in self.valid_types.items():
+                h5s[k] = v
+                self.assertEqual(h5s[k], v)
+                h5s[k] = h5s[k]
+                self.assertEqual(h5s[k], v)
+
+                k_other = k + '-other'
+                h5s[k_other] = h5s[k]
+                self.assertEqual(h5s[k], v)
+                self.assertEqual(h5s[k_other], v)
+                self.assertEqual(h5s[k], h5s[k_other])
+
+    def test_assign_valid_types_within_same_file(self):
+        with self.open_h5store() as h5s:
+            with self.open_h5store() as same_h5s:
+                for k, v in self.valid_types.items():
+                    h5s[k] = v
+                    self.assertEqual(h5s[k], v)
+                    try:
+                        same_h5s[k] = h5s[k]
+                    except ClosedH5StoreError:
+                        pass
+                    self.assertEqual(h5s[k], v)
+                    self.assertEqual(same_h5s[k], v)
+                    self.assertEqual(h5s[k], same_h5s[k])
+
+    def test_assign_valid_types_between_files(self):
+        with self.open_h5store() as h5s:
+            with self.open_other_h5store() as other_h5s:
+                for k, v in self.valid_types.items():
+                    h5s[k] = v
+                    self.assertEqual(h5s[k], v)
+                    other_h5s[k] = h5s[k]
+                    self.assertEqual(h5s[k], v)
+                    self.assertEqual(other_h5s[k], v)
+                    self.assertEqual(h5s[k], other_h5s[k])
+
     def test_write_invalid_type(self):
         class Foo(object):
             pass
@@ -438,12 +488,23 @@ class H5StorePandasDataTest(H5StoreTest):
             numpy.random.rand(8, size), index=[string.ascii_letters[i] for i in range(8)])
 
     def assertEqual(self, a, b):
-        try:
-            return (a == b).all()
-        except (AttributeError, ValueError):
-            return super(H5StorePandasDataTest, self).assertEqual(a, b)
+        if isinstance(a, Mapping):
+            assert isinstance(b, Mapping)
+            if six.PY2:
+                super(H5StorePandasDataTest, self).assertEqual(
+                    sorted(map(str, a.keys())),
+                    sorted(map(str, b.keys())))
+            else:
+                super(H5StorePandasDataTest, self).assertEqual(a.keys(), b.keys())
+            for key in a:
+                self.assertEqual(a[key], b[key])
         else:
-            assert isinstance(a, pandas.DataFrame)
+            try:
+                return (a == b).all()
+            except (AttributeError, ValueError):
+                return super(H5StorePandasDataTest, self).assertEqual(a, b)
+            else:
+                assert isinstance(a, pandas.DataFrame)
 
 
 @unittest.skipIf(not PANDAS_AND_TABLES, 'requires pandas and pytables')
@@ -456,22 +517,6 @@ class H5StoreNestedPandasDataTest(H5StorePandasDataTest):
         return dict(df=pandas.DataFrame(
             numpy.random.rand(8, size), index=[string.ascii_letters[i] for i in range(8)]))
 
-    def assertEqual(self, a, b):
-        try:
-            super(H5StoreNestedPandasDataTest, self).assertEqual(len(a), len(b))
-            if six.PY2:
-                super(H5StoreNestedPandasDataTest, self).assertEqual(
-                    list(map(str, sorted(a.keys()))),
-                    list(map(str, sorted(b.keys()))))
-            else:
-                super(H5StoreNestedPandasDataTest, self).assertEqual(a.keys(), b.keys())
-            for key in a:
-                super(H5StoreNestedPandasDataTest, self).assertEqual(a[key], b[key])
-        except (TypeError, AttributeError):
-            super(H5StoreNestedPandasDataTest, self).assertEqual(a, b)
-        else:
-            assert isinstance(a, Mapping) and isinstance(b, Mapping)
-
 
 class H5StoreMultiThreadingTest(BaseH5StoreTest):