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

xarray.rolling_window Converts dims Argument from Tuple to List Causing Issues for Cupy-Xarray #7890

Closed
negin513 opened this issue Jun 2, 2023 · 6 comments · Fixed by #7938
Labels
bug contrib-good-first-issue topic-arrays related to flexible array support

Comments

@negin513
Copy link
Contributor

negin513 commented Jun 2, 2023

What is your issue?

Hello,
I'm currently working on the development of cupy_xarray, and I've come across an issue with xarray.rolling_window. Specifically, it seems that the dims argument gets automatically converted to a list from a tuple.

While numpy.normalize_axis_tuple can handle lists, thereby not raising errors when using numpy-backed xarray objects, its cupy equivalent (cupy.core.internal._normalize_axis_indices) does not. When a list is passed, it raises the following error:

File cupy/core/internal.pyx:424, in cupy.core.internal._normalize_axis_indices()
TypeError: 'list' object cannot be interpreted as an integer.

It seems the line that might fix this is L2487:

xarray/xarray/core/variable.py

Lines 2487 to 2494 in c9d89e2

axis = [self.get_axis_num(d) for d in dim]
new_dims = self.dims + tuple(window_dim)
return Variable(
new_dims,
duck_array_ops.sliding_window_view(
padded.data, window_shape=window, axis=axis
),
)

For the time being, we've managed to devise a workaround in cupy, but I thought it would be worthwhile to bring this to your attention.

Relevant Cupy PR: cupy/cupy#7575
Contributor: @dcherian

@negin513 negin513 added the needs triage Issue that has not been reviewed by xarray team member label Jun 2, 2023
@welcome
Copy link

welcome bot commented Jun 2, 2023

Thanks for opening your first issue here at xarray! Be sure to follow the issue template!
If you have an idea for a solution, we would really welcome a Pull Request with proposed changes.
See the Contributing Guide for more.
It may take us a while to respond here, but we really value your contribution. Contributors like you help make xarray better.
Thank you!

@dcherian
Copy link
Contributor

dcherian commented Jun 2, 2023

This seems like a real easy fix?

axis = tuple(self.get_axis_num(d) for d in dim)

EDIT: the Array API seems to type axis as Optional[Union[int, Tuple[int, ...]]] pretty consistently, so it seems like we should always pass tuples down to the array library

@dcherian dcherian added bug topic-arrays related to flexible array support and removed needs triage Issue that has not been reviewed by xarray team member labels Jun 2, 2023
@negin513
Copy link
Contributor Author

negin513 commented Jun 2, 2023

@dcherian : agreed! But I am afraid it might break other components.
Although numpy seems to be able to handle both tuple and list in normalize_axis_tuple and I cannot see any other issues rising from this: https://github.com/numpy/numpy/blob/f67467c21a1797becde3097661996f60df4080ff/numpy/core/numeric.py#L1328

@dcherian
Copy link
Contributor

dcherian commented Jun 2, 2023

I think the only other one is dask, which should also work.

@headtr1ck
Copy link
Collaborator

@negin513 would you mind creating a PR for this?

@negin513
Copy link
Contributor Author

@headtr1ck Thanks. I created the corresponding PR for this : #7938

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

Successfully merging a pull request may close this issue.

3 participants