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

Do we need to update AbstractArray for duck arrays? #6845

Open
4 tasks
dcherian opened this issue Jul 28, 2022 · 6 comments
Open
4 tasks

Do we need to update AbstractArray for duck arrays? #6845

dcherian opened this issue Jul 28, 2022 · 6 comments
Labels
bug topic-arrays related to flexible array support

Comments

@dcherian
Copy link
Contributor

dcherian commented Jul 28, 2022

What happened?

I'm calling cupy.round on a DataArray wrapping a cupy array and it raises an error here:

def __array__(self: Any, dtype: DTypeLike = None) -> np.ndarray:
return np.asarray(self.values, dtype=dtype)

Traceback below:

--> 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

xarray v2022.6.0

cupy 10.6.0

@dcherian dcherian added bug topic-arrays related to flexible array support labels Jul 28, 2022
@Illviljan
Copy link
Contributor

I believe so. The other ones that uses .values will fail as well with sparse.

@keewis
Copy link
Collaborator

keewis commented Jul 28, 2022

Not sure, but maybe cupy.round is not supposed to be called on anything other than cupy (or numpy.ndarray)? If I understand correctly, it is converting everything to cupy (or numpy) using _core.array before actually doing the computation.

Also, my impression of __array__ is that it should only be used to convert custom objects to np.ndarray (usually using np.array or np.asarray)

@dcherian
Copy link
Contributor Author

cupy.round is not supposed to be called on anything other than cupy (or numpy.ndarray

Yeah I'm not sure what the expectation is but I was calling cp.round on a DataArray that wrapped a cupy array. Which is why the __array__ triggered an error. I'll update the first post to clarify.

my impression of array is that it should only be used to convert custom objects to np.ndarray (usually using np.array or np.asarray)

OK that would suggest that the current behaviour is correct

@jakirkham
Copy link

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
Copy link
Member

My understanding is that if __array_function__ is working correctly you should never need to call cupy.round on your dataarray. Instead you should always be able to call np.round and trust that the __array_function__ implementation will dispatch to cupy's equivalent of round automatically.

Is there actually a case where we need the library-specific version of a numpy function to work too?

Do we need to update AbstractArray.array to return the underlying duck array instead of always a numpy array?

(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
Copy link
Contributor Author

My understanding is that if array_function is working correctly you should never need to call cupy.round on your dataarray. Instead you should always be able to call np.round and trust that the array_function implementation will dispatch to cupy's equivalent of round automatically.

👍 Question is whether we are expected to also make cp.round work.

Is there actually a case where we need the library-specific version of a numpy function to work too?

Someone's going to try it =) . At least we should document what's expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug topic-arrays related to flexible array support
Projects
None yet
Development

No branches or pull requests

5 participants