-
Notifications
You must be signed in to change notification settings - Fork 30
support inplace ops in array_ufunc #394
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
Conversation
dpctl/dptensor/numpy_usm_shared.py
Outdated
@@ -78,6 +78,15 @@ def _get_usm_base(ary): | |||
return None | |||
|
|||
|
|||
def convert_ndarray_to_np_ndarray(x): | |||
if isinstance(x, ndarray): | |||
return np.ndarray(x.shape, x.dtype, x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is sufficient. Is this doing the right thing for non-contiguous array x
?
In [1]: import numpy as np
In [2]: X = np.empty((7, 5))
In [3]: Y = X[1::2, -1::-2]
In [4]: Y.flags
Out[4]:
C_CONTIGUOUS : False
F_CONTIGUOUS : False
OWNDATA : False
WRITEABLE : True
ALIGNED : True
WRITEBACKIFCOPY : False
UPDATEIFCOPY : False
In [5]: np.ndarray(Y.shape, Y.dtype, Y)
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
<ipython-input-5-86e1bd43f835> in <module>
----> 1 np.ndarray(Y.shape, Y.dtype, Y)
ValueError: ndarray is not contiguous
This code will need to change to use np.array(x, copy=False, subok=False)
.
In [8]: Z = np.array(Y, copy=False, subok=False)
In [10]: (Z.strides , Y.strides)
Out[10]: ((80, -16), (80, -16))
In [11]: (Z.shape , Y.shape)
Out[11]: ((3, 3), (3, 3))
In [12]: (Y.dtype, X.dtype)
Out[12]: (dtype('float64'), dtype('float64'))
In [15]: Z.ctypes.data
Out[15]: 94827342751896
In [16]: Y.ctypes.data
Out[16]: 94827342751896
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oleksandr-pavlyk Thanks again for noticing the potential issue. There is a bigger issue here I think. If np.array does make a copy and this is used for the out argument, then it is the copy that would be updated and not the original, right? There's a line about 20 above in the source code where for non-out arguments the conversion to np.ndarray is done and this one should also use your np.array idea but we don't have the issue there where there is a copy because it is input-only. Do you think that is right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps for out
keyword arguments we have to post-check that for y = np.array(x, copy=False, subok=False)
we have x
is an ndarray
subclass and x.ctypes.data == y.ctypes.data
and raise an error if it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I think we must insist that the out
be only ndarray
and raise an error otherwise, sort of like NumPy already does:
In [1]: import numpy as np
In [2]: np.sin([1.,2.,3.], out=[1.,2.,3.])
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-2-d866583ee0d2> in <module>
----> 1 np.sin([1.,2.,3.], out=[1.,2.,3.])
TypeError: return arrays must be of ArrayType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I think we must insist that the
out
be onlyndarray
and raise an error otherwise, sort of like NumPy already does:In [1]: import numpy as np In [2]: np.sin([1.,2.,3.], out=[1.,2.,3.]) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-2-d866583ee0d2> in <module> ----> 1 np.sin([1.,2.,3.], out=[1.,2.,3.]) TypeError: return arrays must be of ArrayType
@oleksandr-pavlyk My understanding is that for inplace operations you get a tuple of (np.ndarray,) but for non in-place operations, we get a non-tuple np.ndarray. Is that your understanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No exactly. For out-of-place operations out=None
, for in-place, it is either an array, or a tuple of arrays of required length:
In [1]: import numpy as np
In [2]: res = np.empty((4,))
In [3]: X = np.array([1., 1.1, 1.2, 1.3])
In [4]: np.sin(X, out=res)
Out[4]: array([0.84147098, 0.89120736, 0.93203909, 0.96355819])
In [5]: res
Out[5]: array([0.84147098, 0.89120736, 0.93203909, 0.96355819])
In [6]: np.sin(2*X, out=(res,))
Out[6]: array([0.90929743, 0.8084964 , 0.67546318, 0.51550137])
In [7]: res
Out[7]: array([0.90929743, 0.8084964 , 0.67546318, 0.51550137])
There are u-functions in NumPy that return more than one argument, e.g. np.frexp
. For them
In [10]: X = np.array([3.4, 1.2])
In [11]: mant = np.empty((2,), dtype='d')
In [12]: exp = np.empty((2,), dtype='i4')
In [13]: np.frexp(X, out=(mant, exp))
Out[13]: (array([0.85, 0.6 ]), array([2, 1], dtype=int32))
In [14]: mant
Out[14]: array([0.85, 0.6 ])
In [15]: exp
Out[15]: array([2, 1], dtype=int32)
Running this branch locally:
also, running a local example, the code runs into an infinite recursion, but I have not triaged to pinpoint the cause:
|
I fixed issues I have found and am about to push a fix |
…n PR. Added few more tests pertaining to out keyword use
ad97d07
to
ada9420
Compare
@oleksandr-pavlyk Thanks Sasha. |
No description provided.