Add visitor pattern methods #122

jakirkham · 2017-02-22T19:32:29Z

Fixes https://github.com/alimanfoo/zarr/issues/92

Provides visit and visititem that behave identically to their h5py counterparts. This should make it easier to traverse the full hierarchy of a Zarr group. Also should make it easier for users with h5py code to try out using Zarr with fewer changes.

jakirkham · 2017-02-22T19:34:21Z

Only has doctests ATM. Could use some unit tests as well. That said, please let me know what you think.

alimanfoo

Looks good to me. Only very minor suggestions.

alimanfoo · 2017-02-22T22:49:57Z

zarr/hierarchy.py

+
+        Examples
+        --------
+        >>> from __future__ import print_function, unicode_literals


I don't think this is needed in the example. For Zarr docstrings are standardised to PY3(6).

Yeah, I figured it couldn't hurt.

alimanfoo · 2017-02-22T22:51:15Z

zarr/hierarchy.py

+        >>> g2 = g1.create_group('foo')
+        >>> g3 = g1.create_group('bar')
+        >>> d1 = g1.create_dataset('baz', shape=100, chunks=10, fill_value=0)
+        >>> d2 = g1.create_dataset('quux', shape=200, chunks=20, fill_value=0)


Maybe use 'foo/quux' as the dataset name or something like that, to show what happens when you have a hierarchy?

We can do that. Was thinking about doing something similar. Though the tests explore this as well.

May also drop datasets (or at least disable compression) as the compression levels seem to vary, which would make the doctest annoying to keep.

alimanfoo · 2017-02-22T22:51:52Z

zarr/hierarchy.py

+        >>> g2 = g1.create_group('foo')
+        >>> g3 = g1.create_group('bar')
+        >>> d1 = g1.create_dataset('baz', shape=100, chunks=10, fill_value=0)
+        >>> d2 = g1.create_dataset('quux', shape=200, chunks=20, fill_value=0)


Again suggest to have an example showing what happens with a hierarchy, e.g., 'foo/quux'.

xref: https://github.com/alimanfoo/zarr/pull/122#discussion_r102634014

alimanfoo · 2017-02-22T22:56:38Z

zarr/hierarchy.py

@@ -414,6 +416,78 @@ def arrays(self):
                                 chunk_store=self._chunk_store,
                                 synchronizer=self._synchronizer)

+    def visit(self, func):
+        """Run callable on each object name.


Suggest to replace callable with `func`. Also maybe worth adding a note about how the return value is handled, e.g., from h5py source: "Returning None continues iteration; returning anything else aborts iteration and returns that value."

alimanfoo · 2017-02-22T22:57:42Z

zarr/hierarchy.py

+                return value
+
+    def visititems(self, func):
+        """Run callable on each object name and object.


As for visit(), suggest to replace callable with `func` and adding a note about how the return value is handled.

Note to self: https://github.com/alimanfoo/zarr/pull/122#discussion_r102596944

alimanfoo · 2017-02-22T23:00:52Z

zarr/hierarchy.py

+
+        Examples
+        --------
+        >>> from __future__ import print_function, unicode_literals


Not needed I don't think.

Same as above.

alimanfoo · 2017-02-22T23:05:16Z

zarr/hierarchy.py

+        def _visit(obj, func=func):
+            keys = sorted(getattr(obj, "keys", lambda : [])())
+            for each_key in keys:
+                for value in _visit(obj[each_key], func=func):


Think there's a call to func(each_key) missing here?

Yeah, I was playing with stuff and pushed a bad commit. Unfortunately have been AFK until now. Should be fixed.

alimanfoo · 2017-02-22T23:06:47Z

zarr/hierarchy.py

+
+        def _visit(obj, func=func):
+            keys = sorted(getattr(obj, "keys", lambda : [])())
+            for each_key in keys:


Missing call to func(each_key, obj[each_key])?

Same as above.

jakirkham · 2017-02-23T05:29:04Z

Ok, addressed comments and fixed bugs. This is ok on my end.

alimanfoo · 2017-02-23T09:58:56Z

Something weird happening with CI, maybe related to install of appdirs? Try upgrading to appdirs==1.4.1 in requirements_dev.txt? Or maybe remove appdirs altogether from requirements_dev.txt as it's probably not important to pin it, and let latest get brought in by whichever package requires it?

jakirkham · 2017-02-23T14:24:51Z

Yeah, this cropped up with the release of 1.4.1. Have raised upstream to see if they have any idea.

xref: ActiveState/appdirs#89

jakirkham · 2017-02-23T14:42:49Z

Put together PR ( https://github.com/alimanfoo/zarr/pull/124 ) to try and fix the CI issues.

alimanfoo

Couple of small things, otherwise looks good.

alimanfoo · 2017-02-23T15:59:47Z

zarr/hierarchy.py

+
+        Examples
+        --------
+        >>> from __future__ import print_function, unicode_literals


Could you remove this, just for consistency with other docstrings.

Sure, done.

alimanfoo · 2017-02-23T16:00:11Z

zarr/hierarchy.py

+
+        Examples
+        --------
+        >>> from __future__ import print_function, unicode_literals


Same as above.

alimanfoo · 2017-02-23T16:04:20Z

zarr/hierarchy.py

+
+        """
+
+        def _visit(obj):


This is now a duplicate of _visit above. Suggest to factor out into a single private function.

Yep, was actually thinking of refactoring it differently though. I think we could have a visitvalues function that both visit and visititems use. Have a WIP branch for this, but it isn't quite done yet.

Have pushed a refactoring using visitvalues, which seems to work nicely.

jakirkham · 2017-02-23T17:46:33Z

Also made changes so that all of these will show up in the docs. Built the docs locally to make sure.

alimanfoo · 2017-02-23T23:13:40Z

Thanks for the changes, looks good.

So I'm just reading the h5py docs, and apparently the name passed into the visiting function "will be the name of the object relative to the current group". As currently coded, this PR will pass the object path, which is the name relative to the root group. I guess if we are aiming for h5py compatibility where possible then we need to follow the same behaviour? Unless you have a reason to do it differently?

jakirkham · 2017-02-23T23:35:39Z

Just to make sure we are on the same page, by current group, I believe they mean the group we called visit* on. At least that is what the h5py's visit* methods seem to do.

That's a good point. Thanks for raising it. This was definitely an oversight on my part. Not intentional.

Do you have any suggestions for how we should do this? If not, I'm thinking we could just strip out that first x characters that correspond to the parent group path or replace the first match.

alimanfoo · 2017-02-23T23:42:22Z

No problem, yes that's how I read the docs, name is supposed to be relative to the group we called visit* on. I think it would be fine to do something like slice the .path property of the object being visited, using the length of the .path property on the group we called visit* on as the slice start coordinate.

Provides `visit` and `visititem` that behave identically to their h5py counterparts. This should make it easier to traverse the full hierarchy of a Zarr group. Also should make it easier for users with h5py code to try out using Zarr with fewer changes.

This method seems to be at the core of `visit` and `visititems`. So it makes sense to refactor it out. Though it also provides usable functionality of its own. So it makes sense to expose it as part of the API too.

jakirkham · 2017-02-24T01:10:04Z

Alright, I have now updated the visit and visititems functions to provide relative paths. Added an example of this to the documentation. Also included a bunch of related unit tests. Hopefully that addresses it.

alimanfoo · 2017-02-24T13:52:54Z

Great, thank you!

jakirkham · 2017-02-24T14:27:38Z

Thanks for the good review.

jakirkham mentioned this pull request Feb 22, 2017

Visitor Pattern #92

Closed

alimanfoo reviewed Feb 22, 2017

View reviewed changes

jakirkham changed the title ~~WIP: Add visitor pattern methods~~ Add visitor pattern methods Feb 23, 2017

jakirkham mentioned this pull request Feb 23, 2017

ENH: copy method #113

Closed

13 tasks

jakirkham mentioned this pull request Feb 23, 2017

Fix CI #124

Merged

alimanfoo reviewed Feb 23, 2017

View reviewed changes

jakirkham added 3 commits February 23, 2017 19:26

Refactor out visitvalues.

ab37a6d

This method seems to be at the core of `visit` and `visititems`. So it makes sense to refactor it out. Though it also provides usable functionality of its own. So it makes sense to expose it as part of the API too.

Add visitkeys as an alias to visit.

143954a

alimanfoo merged commit f8da93a into zarr-developers:master Feb 24, 2017

alimanfoo modified the milestone: v2.2 Feb 24, 2017

jakirkham deleted the add_visitor_patterns branch February 24, 2017 14:27

alimanfoo mentioned this pull request Oct 24, 2017

v2.2 release notes #159

Closed

4 tasks

alimanfoo added enhancement New features or improvements release notes done Automatically applied to PRs which have release notes. labels Nov 20, 2017

Add visitor pattern methods #122

Add visitor pattern methods #122

Conversation

jakirkham commented Feb 22, 2017

jakirkham commented Feb 22, 2017

alimanfoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Feb 23, 2017

alimanfoo commented Feb 23, 2017

jakirkham commented Feb 23, 2017

jakirkham commented Feb 23, 2017

alimanfoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Feb 23, 2017

alimanfoo commented Feb 23, 2017

jakirkham commented Feb 23, 2017

alimanfoo commented Feb 23, 2017

jakirkham commented Feb 24, 2017

alimanfoo commented Feb 24, 2017

jakirkham commented Feb 24, 2017