Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enables inplace division (__itruediv__) for pyopencl.array.Array class. #266

Merged
merged 12 commits into from
Sep 9, 2020

Conversation

maweigert
Copy link

I noticed that currently the division-assignment operator of the Array class ('/=') is allocating new memory for the result Array. I therefore added a respective inplace version, similar to the already inplace multiplication (imul).

This is similar to already inplace multiplication (__imul__).
@inducer
Copy link
Owner

inducer commented Feb 10, 2019

Thanks! Could you add a test?

@inducer
Copy link
Owner

inducer commented Feb 12, 2019

- raise exception if array cannot be cast to result type
- enable inplace division of integer types
@inducer
Copy link
Owner

inducer commented Feb 12, 2019

@maweigert
Copy link
Author

Sorry for the formatting issues - should be fixed now.

Btw: When diving an integer array x by a scalar integer s, currently the inverse scalar 1/s is cast to the integer before the division (and not after the division), leading to the result zeroing out. As the division kernel would cast the result anyway to the int type, that (casting 1/s) seems not necessary, or? I removed it for the in-place division and it seems to work just fine ("out-place" is the current way):

import pyopencl as cl
import pyopencl.array as cl_array
import numpy as np

ctx = cl.create_some_context(interactive = False)
queue = cl.CommandQueue(ctx)

x = np.array([1,2,3,4,5], np.uint16)
x_g = cl_array.to_device(queue, x)
s = 2

y_g = x_g / s
x_g /= s

print("numpy     ", (x/s).astype(np.uint16))
print("out-place ", y_g.get())
print("in-place  ", x_g.get())
numpy      [0 1 1 2 2]
out-place  [0 0 0 0 0]
in-place   [0 1 1 2 2]

self._div(self, self, other))
else:
if other == 1:
return self.copy()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the copy?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I overlooked that. It should return of course self.

@inducer
Copy link
Owner

inducer commented Feb 12, 2019

With respect to division behavior, I think the right semantics would be to match numpy's behavior as closely as possible, particularly for anything truediv:

>>> import numpy as np
>>> np.array([1,2,3,4,5], dtype=np.int32)/3
array([0.33333333, 0.66666667, 1.        , 1.33333333, 1.66666667])

But it looks like that's not at all the case for now. (#268) Could you fix __itruediv__ to do the right thing? (And, if you're feeling generous, __truediv__ as well?) Thanks!

@maweigert
Copy link
Author

maweigert commented Feb 12, 2019

Yes, that would be the best. For out-of place that would mean to upcast the result. For in-place, this would not be defined, so __itruediv__ now raises an error if a cast would be necessary, similar to what numpy does:

>>> import numpy as np
>>> x = np.array([1,2,3], np.uint16)
>>> x /= 2.2

TypeError: ufunc 'true_divide' output (typecode 'd') could not be coerced to provided output parameter (typecode 'B') according to the casting rule ''same_kind''

@maweigert
Copy link
Author

Hi,

The result of in-place and out-of-place division should now match in almost all cases the numpy equivalent (both numerically, and in dtype). Test now include a loop over all dtype combinations of divisors and dividends.

import pyopencl as cl
import pyopencl.array as cl_array
import numpy as np

ctx = cl.create_some_context(interactive = False)
queue = cl.CommandQueue(ctx)

x = np.array([1,2,3,4,5], dtype=np.int32)
y = cl_array.to_device(queue, x)

x  = x / 3 
y  = y / 3

print(x, x.dtype)
print(y, y.dtype)


[0.33333333 0.66666667 1.         1.33333333 1.66666667] float64
[0.33333333 0.66666667 1.         1.33333333 1.66666667] float64

There are still some annoying cases where numpy casts are incosistent between inplace and out-of-place division, e.g.

a = np.ones(1, np.float32);
b = np.ones(1, np.int32)

a /= b
c = a / b
print(a.dtype)
print(c.dtype)

float32
float64

@inducer
Copy link
Owner

inducer commented Feb 12, 2019

This had a bunch of CI failures: https://gitlab.tiker.net/inducer/pyopencl/merge_requests/72/pipelines

@inducer
Copy link
Owner

inducer commented Feb 13, 2019

More CI failures, sorry: https://gitlab.tiker.net/inducer/pyopencl/pipelines/15769

@inducer
Copy link
Owner

inducer commented Sep 9, 2020

Don't know if you're still around/interested, but there's now native CI on Github. I've merged this branch with master.

@maweigert
Copy link
Author

Don't know if you're still around/interested, but there's now native CI on Github. I've merged this branch with master.

Thanks!

@inducer
Copy link
Owner

inducer commented Sep 9, 2020

After all that: LGTM, thanks for your contribution!

@inducer inducer merged commit 5d2e26a into inducer:master Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants