Multimethods for indexing routines #67

joaosferreira · 2020-07-25T22:37:33Z

This PR intends to expand unumpy's coverage of indexing routines. The multimethods added so far are the following:

Generating index arrays

indices
ix_
ravel_multi_index
unravel_index
diag_indices
diag_indices_from
mask_indices
tril_indices
tril_indices_from
triu_indices
triu_indices_from

Some observations:

The failing tests are in the Sparse backend. Because ones creates a COO with a fill_value of 1.0, the call to the next method fails since a zero fill_value is necessary.
I'm working on default implementations for ravel_multi_index and unravel_index. I'll push them as soon as I have a working prototype but tips on how to implement these are welcomed.
There are three classes that should be added as well since they are part of these routines, they are c_, r_ and s_.

Note: I'll be adding more multimethods that target these routines later.

hameerabbasi · 2020-07-27T08:19:11Z

Here's an implementation for ravel_multi_index: https://github.com/pydata/sparse/blob/9dc40e15a04eda8d8efff35dfc08950b4c07a810/sparse/_coo/common.py#L56-L64

And here's the equivalent of unravel_index: https://github.com/pydata/sparse/blob/9dc40e15a04eda8d8efff35dfc08950b4c07a810/sparse/_coo/core.py#L1811-L1815

hameerabbasi · 2020-07-27T08:20:35Z

The failing tests are in the Sparse backend. Because ones creates a COO with a fill_value of 1.0, the call to the next method fails since a zero fill_value is necessary.

xfail the tests for this backend.

joaosferreira · 2020-07-27T18:26:10Z

Some notes regarding the default implementation for ravel_multi_index added in 0981d47:

It iterates through the 2nd dimension of multi_index to apply the corresponding mode to each index since a tuple of modes is supported. There may be a way of doing this by iterating over dims like in this implementation which would be more efficient. If not then this implementation might be discarded because of an expensive loop.
On implementing the order kwarg, when using column-major (Fortran-style) order with 2 dimensions I think you only need to swap rows with columns but for more dimensions I'm not sure.

joaosferreira · 2020-07-27T23:59:38Z

Same as for ravel_multi_index, the default implementation for unravel_index added in bda0ab9 is also missing the order kwarg.

hameerabbasi · 2020-07-28T07:35:30Z

For the order='F' kwarg, one just has to traverse the shape/indices in reverse. If the end-result is multiple indices, then it might also be necessary to flip them around, as you have calculated them with a reversed shape.

joaosferreira · 2020-07-31T03:04:45Z

The multimethods added in the last commit:

Indexing-like operations

take
take_along_axis
choose
diagonal
select

Inserting data into arrays

place
put
put_along_axis
putmask
fill_diagonal

Iterating over arrays

nditer
ndenumerate
ndindex
nested_iters

Some observations:

Should as_stridedbe added? If yes place it in a separate file with a path like unumpy/lib/stride_tricks/_multimethods.py and set the mutimethod's domain to numpy.lib.stride_tricks? Similarly with Arrayterator?
Less related to unumpy but I found that NumPy's insert behaves weirdly when you pass it a list with only one out-of-bounds index to its obj argument. Here is an example:

In [2]: onp.insert([1, 2, 3, 4, 5], [99], -5)                                   
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-5f700fb21b7e> in <module>
----> 1 onp.insert([1, 2, 3, 4, 5], [99], -5)

<__array_function__ internals> in insert(*args, **kwargs)

~/miniconda3/envs/unumpy/lib/python3.7/site-packages/numpy/lib/function_base.py in insert(arr, obj, values, axis)
   4575             raise IndexError(
   4576                 "index %i is out of bounds for axis %i with "
-> 4577                 "size %i" % (obj, axis, N))
   4578         if (index < 0):
   4579             index += N

TypeError: %i format: a number is required, not list

The error message is not correct. Only problem with unumpy is that some of the defaults that use insert might produce this strange error.

The default for fill_diagonal seems rather simple using item assignment but this not viable because of read-only arrays. Can you think of another way of implementing it?
The class flatiter can't be instantiated. What to do with classes such as this one that don't have a __call__ method? Also, it seems that c_, r_, and s_ are called with brackets so I'm guessing that these need a metaclass which overrides __getitem__.
putmask seems very similar to place. Maybe the former's default should be the latter.
Not sure how to fix the failing test for place in the Dask backend. Maybe it needs a custom a default.

Note: Looking into implementing other defaults as well.

hameerabbasi · 2020-08-03T11:00:00Z

hould as_stridedbe added? If yes place it in a separate file with a path like unumpy/lib/stride_tricks/_multimethods.py and set the mutimethod's domain to numpy.lib.stride_tricks? Similarly with Arrayterator?

stride_tricks, probably not. Arrayterator is doable.

Less related to unumpy but I found that NumPy's insert behaves weirdly when you pass it a list with only one out-of-bounds index to its obj argument. Here is an example:

The best path is a good error message.

The default for fill_diagonal seems rather simple using item assignment but this not viable because of read-only arrays. Can you think of another way of implementing it?

It shouldn't have a default, it requires mutability of arrays.

The class flatiter can't be instantiated. What to do with classes such as this one that don't have a __call__ method? Also, it seems that c_, r_, and s_ are called with brackets so I'm guessing that these need a metaclass which overrides __getitem__.

You're correct on the ?_ classes. For flatiter, can you check what NumPy does to instantiate it?

putmask seems very similar to place. Maybe the former's default should be the latter.

~~Correct, although I'd use a lambda to guard it, as the argument names are different.~~ See #67 (comment)

Not sure how to fix the failing test for place in the Dask backend. Maybe it needs a custom a default.

Skip it, Dask arrays are immutable.

@peterbell10 can you check this PR for CuPy?

peterbell10 · 2020-08-03T11:05:53Z

putmask seems very similar to place. Maybe the former's default should be the latter.

Their behaviour is slightly different. putmask is similar to a[mask] = b[mask] whereas place is more like a[mask] = b.

peterbell10 · 2020-08-03T11:55:52Z

unumpy/tests/test_numpy.py

+        (np.fill_diagonal, ([[0, 0], [0, 0]], 1), {}),
+        (np.nditer, ([[1, 2, 3]],), {}),
+        (np.ndenumerate, ([[1, 2], [3, 4]],), {}),
+        (np.ndindex, (3, 2, 1), {}),


This is failing because overridden_class isn't implemented for the cupy backend.

I could try implement it but I will not be able to test it. CI does not report the tests in CuPy, right?

You should be able to just copy from the other backends.

Since CI doesn't complain about CuPy I think I will add overriden_class to it in a separate PR if that's okay with you.

unumpy/_multimethods.py

peterbell10 · 2020-08-03T12:04:13Z

unumpy/tests/test_numpy.py

@@ -326,6 +346,16 @@ def test_functions_coerce_with_dtype(backend, method, args, kwargs):
        (np.dsplit, ([[[1, 2], [3, 4]], [[5, 6], [7, 8]]], 2), {}),
        (np.hsplit, ([[1, 2], [3, 4]], 2), {}),
        (np.vsplit, ([[1, 2], [3, 4]], 2), {}),
+        (np.ix_, ([0, 1], [2, 4]), {}),
+        (np.unravel_index, ([22, 41, 37], (7, 6)), {}),


This needs an xfail. It's broken on cupy's side.

What reason message do you think it's appropriate?

"cupy.unravel_index is broken in version 6.0"

ix_ is working or broken as well?

ix_ works.

Also, what error is being raised by cupy.unravel_index (e.g., ValueError, TypeError)?

TypeError.

joaosferreira · 2020-08-03T14:32:20Z

You're correct on the ?_ classes. For flatiter, can you check what NumPy does to instantiate it?

According to the docs, it's instantiated by x.flat for an array x.

Skip it, Dask arrays are immutable.

By skipping do you mean adding it to the exceptions list, xfail or removing it entirely?

hameerabbasi · 2020-08-03T14:36:39Z

Add it to the exceptions list.

joaosferreira · 2020-08-03T15:10:00Z

Add it to the exceptions list.

Adding it to the list still produces the error. Maybe xfailing it?

hameerabbasi · 2020-08-03T15:12:34Z

It seems most likely a compute_blocks call is needed somewhere.

unumpy/tests/test_numpy.py

joaosferreira · 2020-08-04T15:17:03Z

It seems most likely a compute_blocks call is needed somewhere.

Not sure what you mean.

hameerabbasi · 2020-08-04T15:20:09Z

Not sure what you mean.

My bad, it was compute_chunk_sizes.

unumpy/unumpy/dask_backend.py

Line 82 in 8d1ca5c

nonzero_idxs = unumpy.nonzero(filt)[0].compute_chunk_sizes()

You need the same approach taken for trim_zeros.

joaosferreira · 2020-08-04T15:30:11Z

You need the same approach taken for trim_zeros.

Dask doesn't have delete and copyto which are needed for Dask's implementation of place. Use Numpy's methods instead (not sure if this works)? If not then I think the implementation will just return NotImplemented which is not necessarily an issue.

Edit: Actually if NotImplemented is returned in the custom implementation then unumpy falls back to the default which is what we want to avoid in the first place. In the end maybe we should just xfail it.

hameerabbasi · 2020-08-06T12:11:10Z

unumpy/tests/test_numpy.py

+    if method is np.s_:
+        assert isinstance(ret, slice)
+    else:
+        assert isinstance(ret, onp.ndarray)


That's interesting, do these return numpy.ndarrays for other backends as well? If so, that seems to be a bug in the backends.

These classes are not dispatching their arguments and since the backends don't have them (at least Dask and Sparse) unumpy overrides them to NumPy classes which in turn convert the arguments into numpy.ndarrays. I think that they should dispatch their arguments but either way the NumPy classes will be operating on ndarrays from other backends (it works in some cases). Other option could be to just return NotImplemented.

I will debug this, see what's going wrong, but if what you say is correct, NotImplemented is the way to go.

Maybe create a list of not implemented classes in each backend and inside overridden_class check if self is in that list, if yes then return NotImplemented. I tried dispatching the list-like arguments in __getitem__. The Dask tests pass because of this behaviour:

In [4]: a = da.from_array(onp.asarray([1, 2, 3])) In [5]: res = onp.c_[a] /Users/joaoferreira/miniconda3/envs/unumpy/lib/python3.7/site-packages/dask/array/core.py:1356: FutureWarning: The `numpy.ndim` function is not implemented by Dask array. You may want to use the da.map_blocks function or something similar to silence this warning. Your code may stop working in a future release. FutureWarning, In [6]: res Out[6]: array([[1], [2], [3]]) In [7]: type(res) Out[7]: numpy.ndarray

And Sparse tests fail because of this:

In [8]: a = sparse.as_coo(onp.asarray([1, 2, 3])) In [9]: res = onp.c_[a] --------------------------------------------------------------------------- RuntimeError Traceback (most recent call last) <ipython-input-9-f3f273ef9019> in <module> ----> 1 res = onp.c_[a] ~/miniconda3/envs/unumpy/lib/python3.7/site-packages/numpy/lib/index_tricks.py in __getitem__(self, key) 385 else: 386 item_ndim = ndim(item) --> 387 newobj = array(item, copy=False, subok=True, ndmin=ndmin) 388 if trans1d != -1 and item_ndim < ndmin: 389 k2 = ndmin - item_ndim ~/miniconda3/envs/unumpy/lib/python3.7/site-packages/sparse/_sparse_array.py in __array__(self, **kwargs) 221 if not AUTO_DENSIFY: 222 raise RuntimeError( --> 223 "Cannot convert a sparse array to dense automatically. " 224 "To manually densify, use the todense method." 225 ) RuntimeError: Cannot convert a sparse array to dense automatically. To manually densify, use the todense method.

Even if they all passed I'm guessing that we want to avoid calling NumPy's methods/classes with ndarrays from other backends.

This is fixed by the latest commits. The classes that are not present in a given backend return NotImplemented.

Note: I had to overwrite _unwrapped in the call methods because when the backends returned NotImplemented the classes were defaulting to _unwrapped at the end of the call stack which made them fail.

joaosferreira · 2020-08-14T14:07:46Z

I will fast-forward merge master if that's okay with you. That way I can keep working on this PR knowing that #72 fixes some of its issues.

…ng branch 'master'

joaosferreira · 2020-08-15T12:00:28Z

This should be good to merge now.

hameerabbasi · 2020-08-16T22:41:17Z

Thanks @joaosferreira!

Add multimethods for indexing routines

0a168c9

Add defaut implementation for ravel_multi_index

0981d47

Add default implementation for unravel_index

bda0ab9

Add order kwarg to ravel_multi_index's and unravel_index's defaults

a30f2d4

joaosferreira force-pushed the indexing-routines branch from ad1d7a0 to a30f2d4 Compare July 28, 2020 16:27

Add more multimethods for indexing routines

810e580

Add default implementation for indices

ea0e0fe

joaosferreira marked this pull request as ready for review August 3, 2020 10:48

joaosferreira requested review from hameerabbasi and peterbell10 August 3, 2020 10:48

peterbell10 reviewed Aug 3, 2020

View reviewed changes

Add c_, r_ and s_

57cf4de

Add lib.Arrayterator

b1e875e

joaosferreira commented Aug 3, 2020

View reviewed changes

unumpy/tests/test_numpy.py Outdated Show resolved Hide resolved

joaosferreira added 3 commits August 3, 2020 17:56

Add default implementation for ix_

eaefc2d

Add overriding metaclass for Arrayterator

5c46c50

Add dispatching of choices to choose

3413b30

joaosferreira mentioned this pull request Aug 5, 2020

Add overridden_class to CuPy backend #69

Merged

Add default implementation for putmask

e7aa4f9

joaosferreira force-pushed the indexing-routines branch from c78da8d to e7aa4f9 Compare August 6, 2020 12:04

hameerabbasi reviewed Aug 6, 2020

View reviewed changes

joaosferreira mentioned this pull request Aug 14, 2020

Fix __ua_function__ in backends #72

Merged

joaosferreira added 2 commits August 15, 2020 12:24

Add backend fixes in Quansight-Labs#71 and Quansight-Labs#72 by mergi…

3833cc6

…ng branch 'master'

Add CClass, RClass and IndexExpression

95321f9

hameerabbasi merged commit f5a1d62 into Quansight-Labs:master Aug 16, 2020

joaosferreira deleted the indexing-routines branch August 18, 2020 16:00

joaosferreira added a commit to joaosferreira/unumpy that referenced this pull request Aug 19, 2020

Merge branch 'master' and resolve conflicts with Quansight-Labs#67

8fa8b52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multimethods for indexing routines #67

Multimethods for indexing routines #67

joaosferreira commented Jul 25, 2020

hameerabbasi commented Jul 27, 2020

hameerabbasi commented Jul 27, 2020 •

edited

Loading

joaosferreira commented Jul 27, 2020

joaosferreira commented Jul 27, 2020

hameerabbasi commented Jul 28, 2020

joaosferreira commented Jul 31, 2020

hameerabbasi commented Aug 3, 2020 •

edited

Loading

peterbell10 commented Aug 3, 2020

peterbell10 Aug 3, 2020

joaosferreira Aug 3, 2020

peterbell10 Aug 3, 2020

joaosferreira Aug 3, 2020

peterbell10 Aug 3, 2020

joaosferreira Aug 3, 2020

peterbell10 Aug 3, 2020

joaosferreira Aug 3, 2020

peterbell10 Aug 3, 2020

joaosferreira Aug 3, 2020

peterbell10 Aug 3, 2020

joaosferreira commented Aug 3, 2020

hameerabbasi commented Aug 3, 2020

joaosferreira commented Aug 3, 2020

hameerabbasi commented Aug 3, 2020

joaosferreira commented Aug 4, 2020

hameerabbasi commented Aug 4, 2020

joaosferreira commented Aug 4, 2020 •

edited

Loading

hameerabbasi Aug 6, 2020

joaosferreira Aug 6, 2020

hameerabbasi Aug 6, 2020

joaosferreira Aug 6, 2020

joaosferreira Aug 15, 2020 •

edited

Loading

joaosferreira commented Aug 14, 2020

joaosferreira commented Aug 15, 2020

hameerabbasi commented Aug 16, 2020

Multimethods for indexing routines #67

Multimethods for indexing routines #67

Conversation

joaosferreira commented Jul 25, 2020

hameerabbasi commented Jul 27, 2020

hameerabbasi commented Jul 27, 2020 • edited Loading

joaosferreira commented Jul 27, 2020

joaosferreira commented Jul 27, 2020

hameerabbasi commented Jul 28, 2020

joaosferreira commented Jul 31, 2020

hameerabbasi commented Aug 3, 2020 • edited Loading

peterbell10 commented Aug 3, 2020

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

joaosferreira commented Aug 3, 2020

hameerabbasi commented Aug 3, 2020

joaosferreira commented Aug 3, 2020

hameerabbasi commented Aug 3, 2020

joaosferreira commented Aug 4, 2020

hameerabbasi commented Aug 4, 2020

joaosferreira commented Aug 4, 2020 • edited Loading

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

joaosferreira Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

joaosferreira commented Aug 14, 2020

joaosferreira commented Aug 15, 2020

hameerabbasi commented Aug 16, 2020

hameerabbasi commented Jul 27, 2020 •

edited

Loading

hameerabbasi commented Aug 3, 2020 •

edited

Loading

joaosferreira commented Aug 4, 2020 •

edited

Loading

joaosferreira Aug 15, 2020 •

edited

Loading