Implement `__array_ufunc__` by martindurant · Pull Request #1247 · cupy/cupy

martindurant · 2018-05-17T22:03:39Z

So that numpy functions should work on cupy arrays. Would allow cupy arrays to be used transparently in much numpy-oriented code.

The current plan is to error, if in a non-unary ufunc, a numpy (non-device) and a cupy array are combined.

Based on

Fixes #1230 (eventually)

mrocklin

I'm glad to see this start. Some small comments follow.

Also, to avoid confusion with the term "generalized ufuncs" I recommend changing the title of the PR to something like "implement array_ufunc protocol"

mrocklin · 2018-05-18T14:18:58Z

cupy/core/core.pyx

+        for x in check:
+            if not isinstance(x, (ndarray, Number)):
+                # do *not* pass any old ndarray-like
+                return NotImplemented


Why this extra logic on top of what cupy already does?

Passing to cupy results in
TypeError: Unsupported type <class 'numpy.ndarray'>
or using this as-is results in
TypeError: operand type(s) all returned NotImplemented from __array_ufunc__

Keeping the current means that, in theory, some other implementation could still work in combination with a cupy.ndarray, because NotImplemented implies trying the __array_ufunc__ of the other object involved; but in practice, I think it can only realistically be cupy or a subclass. I thunk best would be to raise a TypeError with a carefully phrased message.

My understanding is that CuPy could also return NotImplemented if they want to give up and hand to some other system and your solution here would just pass that through. Is this correct? If so then this probably isn't the right place to implement this logic. Moreover, I don't think that you and I have enough experience with how CuPy is used to make policy decisions like this. I think that we should avoid this discussion, or perhaps raise it as a separate issue that lives independently from this effort, which I think should be restricted to implementing the __array_ufunc__ protocol.

mrocklin · 2018-05-18T14:19:23Z

cupy/core/core.pyx

+        name = ufunc.__name__
+        if method == '__call__':
+            if ufunc.signature is not None:
+                return NotImplemented


This could use a comment explaining that we don't yet support generalized ufuncs

mrocklin · 2018-05-18T14:22:45Z

cupy/core/core.pyx

+        if 'out' in kwargs:
+            # need to unfold tuple argument in kwargs
+            check = inputs + kwargs['out']
+            kwargs['out'] = kwargs['out'][0]


I'm curious, why was this logic necessary? Also, should we be concerned that if out has length greater than one that we've lost information?

mrocklin · 2018-05-18T14:25:57Z

This could also generally use some tests. I think that this will communicate the intended outcome of this work more clearly to other potential reviewers.

mrocklin · 2018-05-21T14:38:33Z

@martindurant is it ok to remove the WIP label?

martindurant · 2018-05-21T15:00:49Z

I am happy with the code that is here as well as the tests, so yes.
I still need to decide whether to test the add.at() case, or just drop it.

beam2d

Thank you for the PR. I'm excited to support np.xxx functions directly for CuPy. I added some minor comments. I will also ask @okuta for the design direction.

beam2d · 2018-05-22T03:40:30Z

cupy/core/core.pyx

+        import cupy as cp  # top-level ufuncs
+        if 'out' in kwargs:
+            # need to unfold tuple argument in kwargs
+            check = inputs + kwargs['out']


It looks check is not used.

beam2d · 2018-05-22T03:40:59Z

cupy/core/core.pyx

+                # we don't support generalised-ufuncs (gufuncs)
+                return NotImplemented
+            try:
+                cp_ufunc = getattr(cp, ufunc.__name__)


Use name (or remove name).

beam2d · 2018-05-22T03:41:29Z

cupy/core/core.pyx

+                return NotImplemented
+            try:
+                cp_ufunc = getattr(cp, ufunc.__name__)
+                return cp_ufunc(*inputs, **kwargs)


Put this line outside the try clause.

beam2d · 2018-05-22T03:44:51Z

cupy/core/core.pyx

+            # the only ufunc attribute currently
+            # http://docs-cupy.chainer.org/en/stable/reference/ufunc.html#ufunc-at
+            # self.scatter_add(*inputs, **kwargs)
+        else:


Do we have a chance to support some of the reduction routines (like add.reduce via sum, maximum.reduce via amax, etc.) here (maybe via ad-hoc manual mapping)? Or do you think it's better to first support reduce in CuPy's ufuncs (so that no ad-hoc coding is needed here)?

Note that numpy reductions already dispatch if an appropriate method is defined

In [1]: class Foo: ...: def sum(*args, **kwargs): ...: return 123 ...: In [2]: import numpy as np In [3]: np.sum(Foo()) Out[3]: 123

Adding support for ufunc.reduce would also be good, but this seems better to do in the ufuncs themselves rather than in the __array_ufunc__ protocol.

Ah, sum is already dispatched. I misunderstood.

okuta · 2018-05-22T05:06:14Z

cupy/core/core.pyx

+        a number, i.e., raise ValueError instead of silently converting a
+        numpy array.
+        """
+        import cupy as cp  # top-level ufuncs


Please remove as cp, use cupy directly.

okuta · 2018-05-22T05:07:56Z

Thank you for useful PR!

okuta

LGTM except comment

okuta · 2018-05-22T14:20:26Z

cupy/core/core.pyx

+        """
+        import cupy  # top-level ufuncs
+        if 'out' in kwargs:
+            # need to unfold tuple argument in kwargs


Cloud you check tuple length?

As far as I understand, the out tuple could in theory have more entries, for some future ufunc that has multiple outputs. I don't think numpy as any of these, never mind cupy - so I agree that an exception could be raised if the tuple length>1. This is what numpy does, e.g., np.sin(x, out=(x, x)) -> ValueError.

OK, I understood.

okuta · 2018-05-22T14:36:33Z

jenkins, test this please.

This PR will be merge in v5.0.0b2.

mrocklin · 2018-05-23T11:39:25Z

Is there anything more to do here?

okuta · 2018-05-24T00:38:02Z

I found error. Can you fix this?
https://gist.github.com/okuta/fb26f7e8dc30020bb4d9735304fb0d06

martindurant · 2018-05-24T14:23:54Z

@okuta , cupy does not implement the divmod ufunc, so I expect the operation to truly raise an error. However, I cannot figure out what the test is actually testing for, and why it passed before. Has my code actually prevented one of the the previously erroring codepaths from happening?

okuta · 2018-05-25T01:01:55Z

This is message to testing bot.

jenkins, test this please.

beam2d · 2018-05-28T03:08:03Z

@okuta Can I merge it?

okuta · 2018-05-28T04:55:52Z

Please wait. Some tests are failed. I will investigate it.

okuta · 2018-05-28T06:57:05Z

@beam2d Cloud you merge #1286 ?

beam2d · 2018-05-30T08:56:43Z

I merged #1286, so I think this PR is CI ready.
Jenkins, test this please.

martindurant · 2018-05-30T17:11:53Z

I merged in those changes.
It is interesting that coverage showed up, and the patch is 100% covered but the overall coverage apparently went down.

okuta · 2018-06-01T17:16:28Z

jenkins, test this please.

okuta · 2018-06-03T08:23:45Z

tests/cupy_tests/core_tests/test_ndarray_ufunc.py

+
+
+@testing.gpu
+class TestArrayUfunc(unittest.TestCase):


Please add @testing.with_requires('numpy>=1.13') because, test is failed with NumPy<1.13.0.

martindurant · 2018-06-03T17:39:22Z

@okuta , done

okuta · 2018-06-03T22:54:32Z

Thanks!

jenkins, test this please.

okuta · 2018-06-04T01:46:27Z

LGTM!

mrocklin · 2018-06-04T01:48:44Z

I'm very happy to see this merged!

martindurant · 2018-06-04T02:37:12Z

Hurray

ericmjl · 2018-06-04T05:50:35Z

OMG you guys made it happen!!

Martin Durant added 3 commits May 17, 2018 14:53

Add ufunc dunder stob

bc8dcce

Fix for out= keyword

516e448

flake

ce2ba67

mrocklin reviewed May 18, 2018

View reviewed changes

martindurant changed the title ~~WIP: general ufuncs~~ WIP: implement __array__ufunc__ May 18, 2018

martindurant changed the title ~~WIP: implement __array__ufunc__~~ WIP: implement __array_ufunc__ May 18, 2018

Martin Durant added 7 commits May 19, 2018 13:22

Remove explicit type check; add first tests

e2070fb

name tests tests and use fewer fixtures

a5fceac

use arrays

c5572a2

fixes

3f08691

more tests

83a894a

float precision

5b39065

flake

da84060

martindurant changed the title ~~WIP: implement __array_ufunc__~~ implement __array_ufunc__ May 21, 2018

Tidy, remove add.at

915b1c2

okuta added the cat:feature New features/APIs label May 22, 2018

beam2d suggested changes May 22, 2018

View reviewed changes

okuta requested changes May 22, 2018

View reviewed changes

okuta self-assigned this May 22, 2018

okuta added this to the v5.0.0b2 milestone May 22, 2018

fixes from review

941efcf

okuta requested changes May 22, 2018

View reviewed changes

okuta approved these changes May 22, 2018

View reviewed changes

check length of out tuple

fbd829f

beam2d approved these changes May 24, 2018

View reviewed changes

okuta added the st:blocked-by-another-pr Blocked by another pull-request label May 28, 2018

Merge branch 'master' into ufunc

39a384e

okuta removed the st:blocked-by-another-pr Blocked by another pull-request label Jun 3, 2018

okuta requested changes Jun 3, 2018

View reviewed changes

require numpy 1.13

31e8d6f

okuta approved these changes Jun 4, 2018

View reviewed changes

okuta merged commit cb5821c into cupy:master Jun 4, 2018

mrocklin mentioned this pull request Jun 4, 2018

RFC __array_function__ protocol #1322

Closed

martindurant deleted the ufunc branch June 4, 2018 02:37

kmaehashi changed the title ~~implement __array_ufunc__~~ Implement __array_ufunc__ Jun 20, 2018

Uh oh!

Conversation

martindurant commented May 17, 2018

Uh oh!

mrocklin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrocklin commented May 18, 2018

Uh oh!

mrocklin commented May 21, 2018

Uh oh!

martindurant commented May 21, 2018

Uh oh!

beam2d left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okuta commented May 22, 2018

Uh oh!

okuta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okuta commented May 22, 2018

Uh oh!

mrocklin commented May 23, 2018

Uh oh!

okuta commented May 24, 2018

Uh oh!

martindurant commented May 24, 2018

Uh oh!

okuta commented May 25, 2018

Uh oh!

beam2d commented May 28, 2018

Uh oh!

okuta commented May 28, 2018

Uh oh!

okuta commented May 28, 2018

Uh oh!

beam2d commented May 30, 2018

Uh oh!

martindurant commented May 30, 2018

Uh oh!

okuta commented Jun 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martindurant commented Jun 3, 2018

Uh oh!

okuta commented Jun 3, 2018

Uh oh!

beam2d left a comment •

edited

Loading