pydata / xarray

N-D labeled arrays and datasets in Python
https://xarray.dev
Apache License 2.0
3.61k stars 1.08k forks source link

`__array__` copy keyword DeprecationWarnings in `indexing.py` #9400

Open dcamron opened 2 months ago

dcamron commented 2 months ago

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.
welcome[bot] commented 2 months ago

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 commented 2 months ago

I think fixed by https://github.com/pydata/xarray/pull/9393

dopplershift commented 2 months ago

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 commented 2 months ago

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!

dopplershift commented 2 months ago

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 commented 2 months ago

see also: https://github.com/pydata/xarray/blob/6a2eddda8f98007865b0bedb399bdf5f9ab91142/pyproject.toml#L329

keewis commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.