Make Indexer classes not inherit from tuple. #1705

shoyer · 2017-11-09T07:08:27Z

I'm not entirely sure this is a good idea. The advantage is that it ensures that
all our indexing code is entirely explicit: everything that reaches a backend
must be an ExplicitIndexer. The downside is that it removes a bit of
internal flexibility: we can't just use tuples in place of basic indexers
anymore. On the whole, I think this is probably worth it but I would appreciate
feedback.

@fujiisoup can you take a look?

Tests added / passed
Passes git diff upstream/master **/*py | flake8 --diff

I'm not entirely sure this is a good idea. The advantage is that it ensures that all our indexing code is entirely explicit: everything that reaches a backend *must* be an ExplicitIndexer. The downside is that it removes a bit of internal flexibility: we can't just use tuples in place of basic indexers anymore. On the whole, I think this is probably worth it but I would appreciate feedback.

fujiisoup · 2017-11-09T14:19:04Z

This looks pretty clean and less error-prone.

For more cleanliness, I'm wondering if we could more clearly distinguish between raw array-wrappers (such as NumpyIndexingAdapter) and wrappers of array-wrapper (such as MemoryCachedArray).
But as a whole, I like this idea.

Regarding the more array-type support in the future (as suggested in comment),
is there something to prepare in this PR?
I guess there are some typical indexing types, such as Fortran-type and Numpy-type.
Can we have some abstract classes (maybe too early)?

shoyer · 2017-11-09T17:51:40Z

There's been discussion about abstract classes on the NumPy mailing list. It would be nice to use something standard if possible: https://mail.python.org/pipermail/numpy-discussion/2017-November/thread.html#77312

…

On Thu, Nov 9, 2017 at 6:19 AM Keisuke Fujii ***@***.***> wrote: This looks pretty clean and less error-prone. For more cleanliness, I'm wondering if we could more clearly distinguish between raw array-wrappers (such as NumpyIndexingAdapter) and wrappers of array-wrapper (such as MemoryCachedArray). But as a whole, I like this idea. Regarding the more array-type support in the future (as suggested in comment <#1617 (comment)>), is there something to prepare in this PR? I guess there are some typical indexing types, such as Fortran-type and Numpy-type. Can we have some abstract classes (maybe too early)? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1705 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKS1tRYI6F0WTmR9ozdBdvy0g6Yi-Ipks5s0wnagaJpZM4QXhhG> .

fujiisoup

Just a few comments.

fujiisoup · 2017-11-12T08:24:58Z

xarray/core/variable.py

@@ -766,6 +769,7 @@ def chunk(self, chunks=None, name=None, lock=False):
            if utils.is_dict_like(chunks):
                chunks = tuple(chunks.get(n, s)
                               for n, s in enumerate(self.shape))
+            data = indexing.WrapImplicitIndexing(data, indexing.OuterIndexer)


Why is this line necessary here? (and only OuterIndexer?)
I think it would be better if we could have a simple comment above.

Added a comment to explain, with a link to the upstream dask issue (dask/dask#2883).

fujiisoup · 2017-11-12T08:28:14Z

xarray/core/indexing.py

+    return key.tuple
+
+
+class WrapImplicitIndexing(utils.NDArrayMixin):


This name sounds like a function which behaves opposite to unwrap_explicit_indexer. Can we change this something like ImplicitIndexedArrayWrapper?
(But as the below comment, I did not yet understand how this wrapper will be used.)

Renamed to ImplicitToExplicitIndexingAdapter, which is more consistent with the "Adapter" names of the other array wrapper classes. It's long but descriptive.

fujiisoup · 2017-11-12T08:48:34Z

xarray/core/indexing.py



-class VectorizedIndexer(IndexerTuple):
-    """ Tuple for vectorized indexing """
+class ExplicitNDArrayMixin(utils.NDArrayMixin, ExplicitlyIndexed):


I like a more explicit name such as ExplicitlyIndexedNDArrayMixin. Is it too long?

That's definitely better. Renamed.

fujiisoup · 2017-11-12T08:59:23Z

xarray/core/indexing.py

+        new_key = []
+        for k in key:
+            if isinstance(k, integer_types):
+                k = int(k)


This looks a good idea over checking an integer type in array wrappers 👍

shoyer

Thanks for your review @fujiisoup !

shoyer · 2017-11-12T19:16:19Z

xarray/core/indexing.py



-class VectorizedIndexer(IndexerTuple):
-    """ Tuple for vectorized indexing """
+class ExplicitNDArrayMixin(utils.NDArrayMixin, ExplicitlyIndexed):


That's definitely better. Renamed.

shoyer · 2017-11-12T19:24:39Z

xarray/core/indexing.py

+    return key.tuple
+
+
+class WrapImplicitIndexing(utils.NDArrayMixin):


Renamed to ImplicitToExplicitIndexingAdapter, which is more consistent with the "Adapter" names of the other array wrapper classes. It's long but descriptive.

shoyer · 2017-11-12T20:18:49Z

xarray/core/variable.py

@@ -766,6 +769,7 @@ def chunk(self, chunks=None, name=None, lock=False):
            if utils.is_dict_like(chunks):
                chunks = tuple(chunks.get(n, s)
                               for n, s in enumerate(self.shape))
+            data = indexing.WrapImplicitIndexing(data, indexing.OuterIndexer)


Added a comment to explain, with a link to the upstream dask issue (dask/dask#2883).

shoyer · 2017-11-13T22:49:39Z

I plan to merge this shortly and issue another release candidate unless any one has objections.

It does occur to me that this could break custom backends, since they will not longer be getting tuples as indexers. @rabernat any ideas for how to minimize that pain?

jhamman · 2017-11-13T23:39:15Z

xarray/core/variable.py

@@ -1261,7 +1270,7 @@ def equals(self, other, equiv=duck_array_ops.array_equiv):
            return (self.dims == other.dims and
                    (self._data is other._data or
                     equiv(self.data, other.data)))
-        except (TypeError, AttributeError):
+        except (TypeError, AttributeError) as e:


it doesn't look like e is ever used.

Good catch, this was introduced in debugging

shoyer · 2017-11-14T03:23:24Z

Actually, looking over @rabernat's custom backend code in xmitgcm, I think that should be OK. This would only be a problem if you pass custom duck arrays into xarray.Variable objects, rather than numpy or dask arrays.

shoyer · 2017-11-14T03:32:49Z

OK, merging. Hopefully we'll catch any issues in the next release candidate!

shoyer added 2 commits November 11, 2017 18:05

Merge branch 'master' into indexers-reorg

5014e12

Add validation to ExplicitIndexer classes

bc9b2ea

fujiisoup reviewed Nov 12, 2017

View reviewed changes

Fix pynio test failure

e64bc89

shoyer mentioned this pull request Nov 12, 2017

Explicitly handle array indexing types in dask.array.from_array dask/dask#2883

Open

shoyer added 2 commits November 12, 2017 12:24

Rename and add comments

e203ec9

flake8

3f9b522

shoyer commented Nov 12, 2017

View reviewed changes

shoyer mentioned this pull request Nov 12, 2017

v0.10 Release #1535

Closed

13 tasks

shoyer added 2 commits November 12, 2017 20:17

Fix windows test failure

9c5318f

typo

00bd0fa

shoyer mentioned this pull request Nov 13, 2017

WIP: Zarr backend #1528

Merged

4 tasks

jhamman reviewed Nov 13, 2017

View reviewed changes

leftover from debugging

53bda8a

shoyer merged commit ac854f0 into pydata:master Nov 14, 2017

fujiisoup mentioned this pull request Nov 17, 2017

Fix unexpected loading after print #1724

Merged

4 tasks

shoyer deleted the indexers-reorg branch November 17, 2017 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Indexer classes not inherit from tuple. #1705

Make Indexer classes not inherit from tuple. #1705

shoyer commented Nov 9, 2017 •

edited

Loading

fujiisoup commented Nov 9, 2017

shoyer commented Nov 9, 2017 via email

fujiisoup left a comment

fujiisoup Nov 12, 2017

shoyer Nov 12, 2017

fujiisoup Nov 12, 2017

shoyer Nov 12, 2017

fujiisoup Nov 12, 2017

shoyer Nov 12, 2017

fujiisoup Nov 12, 2017

shoyer left a comment

shoyer Nov 12, 2017

shoyer Nov 12, 2017

shoyer Nov 12, 2017

shoyer commented Nov 13, 2017

jhamman Nov 13, 2017

shoyer Nov 13, 2017

shoyer commented Nov 14, 2017

shoyer commented Nov 14, 2017

		return key.tuple


		class WrapImplicitIndexing(utils.NDArrayMixin):

Make Indexer classes not inherit from tuple. #1705

Make Indexer classes not inherit from tuple. #1705

Conversation

shoyer commented Nov 9, 2017 • edited Loading

fujiisoup commented Nov 9, 2017

shoyer commented Nov 9, 2017 via email

fujiisoup 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

shoyer 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

shoyer commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Nov 14, 2017

shoyer commented Nov 14, 2017

shoyer commented Nov 9, 2017 •

edited

Loading