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

__array__ copy keyword DeprecationWarnings in indexing.py #9400

Open
dcamron opened this issue Aug 22, 2024 · 9 comments
Open

__array__ copy keyword DeprecationWarnings in indexing.py #9400

dcamron opened this issue Aug 22, 2024 · 9 comments

Comments

@dcamron
Copy link

dcamron commented Aug 22, 2024

What is your issue?

Testing for the MetPy xarray accessor reveals warnings from indexing.py that parallel #9393 / #9312. There seem to be a few __array__ implementations that might need updating here and throughout.

on xarray-main@a04d857 and numpy==2.1,

import numpy as np
import xarray as xr
da = xr.tutorial.open_dataset("air_temperature")["air"]
np.array(da.variable._data)
DeprecationWarning: __array__ implementation doesn't accept a copy keyword, so passing copy=False failed. __array__ must implement 'dtype' and 'copy' keyword arguments.
@dcamron dcamron added the needs triage Issue that has not been reviewed by xarray team member label Aug 22, 2024
Copy link

welcome bot commented Aug 22, 2024

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!

@max-sixty
Copy link
Collaborator

I think fixed by #9393

@dopplershift
Copy link
Contributor

Nope, not fixed by #9393. If you read carefully above, this indicates a problem that parallels the issue fixed by #9393. Also, if you run the sample code shared above, current xarray main will issue the indicated warning.

@max-sixty
Copy link
Collaborator

Nope, not fixed by #9393. If you read carefully above, this indicates a problem that parallels the issue fixed by #9393. Also, if you run the sample code shared above, current xarray main will issue the indicated warning.

Very sorry!

@max-sixty max-sixty reopened this Aug 23, 2024
@dopplershift
Copy link
Contributor

There are 6 __array__() methods defined in xarray/core/indexing.py, and I'm guessing this warning is caused by none of them having a copy argument.

@keewis
Copy link
Collaborator

keewis commented Aug 23, 2024

see also:

"ignore:__array__ implementation doesn't accept a copy keyword, so passing copy=False failed.",

@TomNicholas TomNicholas added topic-indexing bug and removed needs triage Issue that has not been reviewed by xarray team member labels Aug 23, 2024
@keewis
Copy link
Collaborator

keewis commented Aug 28, 2024

in any case, PRs welcome. I tried this a couple of months ago, but I was not so clear on how to deal with typing compat with unreleased numpy versions so didn't complete that, but this should be a pretty straight-forward fix (you would need to remove the warning ignore above, though).

@andrew-s28
Copy link
Contributor

Is this potentially solved by adapting the solution in #9393 to mirror something closer to the solution in Scipy? This would involve adding the following code to the xarray.utils module:

# utils.py
copy_if_needed: Optional[bool]

if np.lib.NumpyVersion(np.__version__) >= "2.0.0":
    copy_if_needed = None
elif np.lib.NumpyVersion(np.__version__) < "1.28.0":
    copy_if_needed = False
else:
    # 2.0.0 dev versions, handle cases where copy may or may not exist
    try:
        np.array([1]).__array__(copy=None)  # type: ignore[call-overload]
        copy_if_needed = None
    except TypeError:
        copy_if_needed = False

then adding the copy parameter to every __array__ method as, e.g.,

# indexing.py
def __array__(self, dtype: np.typing.DTypeLike = None, copy: bool | None = None) -> np.ndarray:
    if not copy:
        copy = copy_if_needed
    # Leave casting to an array up to the underlying array type.
    return np.asarray(self.get_duck_array(), dtype=dtype)

FWIW, I implemented the solution in #9393 within only the AbstractArray class since I simple wasn't aware of all of these other __array__ methods in the indexing.py module. If I had known, I would have probably favored this solution in the first place, since it will have less code duplication.

@dcherian
Copy link
Contributor

Hmmm.. I'm not sure why __array__ is duplicated so much. All of those classes should inherit from a couple of shared classes, so we might be able to delete them and only change a couple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants