Repository navigation
Do we need to update AbstractArray for duck arrays? #6845
Description
What happened?
I'm calling cupy.round on a DataArray wrapping a cupy array and it raises an error here:
Lines 155 to 156 in 3f7cc2d
Traceback below:
Details
--> 25 a = _core.array(a, copy=False)
26 return a.round(decimals, out=out)
27
cupy/_core/core.pyx in cupy._core.core.array()
cupy/_core/core.pyx in cupy._core.core.array()
cupy/_core/core.pyx in cupy._core.core._array_default()
~/miniconda3/envs/gpu/lib/python3.7/site-packages/xarray/core/common.py in __array__(self, dtype)
146
147 def __array__(self: Any, dtype: DTypeLike = None) -> np.ndarray:
--> 148 return np.asarray(self.values, dtype=dtype)
149
150 def __repr__(self) -> str:
~/miniconda3/envs/gpu/lib/python3.7/site-packages/xarray/core/dataarray.py in values(self)
644 type does not support coercion like this (e.g. cupy).
645 """
--> 646 return self.variable.values
647
648 @values.setter
~/miniconda3/envs/gpu/lib/python3.7/site-packages/xarray/core/variable.py in values(self)
517 def values(self):
518 """The variable's data as a numpy.ndarray"""
--> 519 return _as_array_or_item(self._data)
520
521 @values.setter
~/miniconda3/envs/gpu/lib/python3.7/site-packages/xarray/core/variable.py in _as_array_or_item(data)
257 TODO: remove this (replace with np.asarray) once these issues are fixed
258 """
--> 259 data = np.asarray(data)
260 if data.ndim == 0:
261 if data.dtype.kind == "M":
cupy/_core/core.pyx in cupy._core.core.ndarray.__array__()
TypeError: Implicit conversion to a NumPy array is not allowed. Please use `.get()` to construct a NumPy array explicitly.
What did you expect to happen?
Not an error? I'm not sure what's expected
np.round(dataarray) does actually work successfully.
My question is : Do we need to update AbstractArray.__array__ to return the underlying duck array instead of always a numpy array?
Minimal Complete Verifiable Example
No response
MVCE confirmation
- Minimal example — the example is as focused as reasonably possible to demonstrate the underlying issue in xarray.
- Complete example — the example is self-contained, including all data and the text of any traceback.
- Verifiable example — the example copy & pastes into an IPython prompt or Binder notebook, returning the result.
- New issue — a search of GitHub Issues suggests this is not a duplicate.
Relevant log output
No response
Anything else we need to know?
No response
Environment
Details
xarray v2022.6.0
cupy 10.6.0
Activity
Illviljan commentedon Jul 28, 2022
I believe so. The other ones that uses
.valueswill fail as well with sparse.keewis commentedon Jul 28, 2022
Not sure, but maybe
cupy.roundis not supposed to be called on anything other thancupy(ornumpy.ndarray)? If I understand correctly, it is converting everything tocupy(ornumpy) using_core.arraybefore actually doing the computation.Also, my impression of
__array__is that it should only be used to convert custom objects tonp.ndarray(usually usingnp.arrayornp.asarray)dcherian commentedon Jul 28, 2022
Yeah I'm not sure what the expectation is but I was calling
cp.roundon a DataArray that wrapped a cupy array. Which is why the__array__triggered an error. I'll update the first post to clarify.OK that would suggest that the current behaviour is correct
jakirkham commentedon Jul 28, 2022
Probably out of my depth here (so please forgive me), but one thing that might be worth looking at is Array API support, which CuPy 10+ supports and Dask is working on support for ( dask/dask#8750 ). Believe XArray is taking some initial steps in this direction recently ( #6804 ), but could easily be misunderstanding the scope/intended usage of the changes there.
TomNicholas commentedon Jul 29, 2022
My understanding is that if
__array_function__is working correctly you should never need to callcupy.roundon your dataarray. Instead you should always be able to callnp.roundand trust that the__array_function__implementation will dispatch to cupy's equivalent ofroundautomatically.Is there actually a case where we need the library-specific version of a numpy function to work too?
(Having said all that we might still want to make this change anyway, this was just an argument for the current behaviour being "good enough".)
dcherian commentedon Jul 29, 2022
👍 Question is whether we are expected to also make
cp.roundwork.Someone's going to try it =) . At least we should document what's expected.