avoid a couple of warnings in `polyfit` #8939

keewis · 2024-04-13T11:49:13Z

towards ⚠️ Nightly upstream-dev CI failed ⚠️ #8844

replace numpy.core.finfo with numpy.finfo
add dtype and copy parameters to all definitions of __array__

The `copy` kwarg appears to only exist in `numpy>=2`.

keewis · 2024-04-14T09:43:37Z

xarray/namedarray/_typing.py

-    def __array__(self, dtype: None = ..., /) -> np.ndarray[Any, _DType_co]: ...
+    def __array__(
+        self, dtype: None = ..., copy: None = ..., /
+    ) -> np.ndarray[Any, _DType_co]: ...

    @overload
-    def __array__(self, dtype: _DType, /) -> np.ndarray[Any, _DType]: ...
+    def __array__(self, dtype: _DType, copy: bool, /) -> np.ndarray[Any, _DType]: ...

    def __array__(
-        self, dtype: _DType | None = ..., /
+        self, dtype: _DType | None = ..., copy: bool | None = None, /


@Illviljan (or anyone else), I might need some help to get the typing right... basically, __array__ is supposed to have a new kwarg, copy, which is either a bool or None.

Edit: my best guess, adding copy: bool | None = None to the overloads, didn't work

There's more places with __array__ to fix:

xarray/core/indexing.py:472: note: Got: xarray/core/indexing.py:472: note: @overload xarray/core/indexing.py:472: note: def __array__(self, None = ..., /) -> ndarray[Any, dtype[generic]] xarray/core/indexing.py:472: note: @overload xarray/core/indexing.py:472: note: def [_DType <: dtype[Any]] __array__(self, _DType, /) -> ndarray[Any, _DType]

numpy's type hints:

@overload def __array__( self, dtype: None = ..., /, *, copy: None | bool = ... ) -> ndarray[Any, _DType_co]: ... @overload def __array__( self, dtype: _DType, /, *, copy: None | bool = ... ) -> ndarray[Any, _DType]: ...

https://github.com/numpy/numpy/blob/ab7649fe2ed8f0f0260322d66631b8dfab57deff/numpy/__init__.pyi#L1466-L1473

keewis · 2024-04-14T10:09:24Z

I've removed all the forwarding of copy to asarray since that doesn't have the kwarg until numpy>=2.

If we can get the type checking to pass this should be ready for reviews and merging.

keewis · 2024-04-14T17:28:26Z

okay, so I believe I've fixed most of the issues. However, since the copy kwarg is new in numpy>=2, mypy will fail to type check properly for versions of numpy<2. I have no idea how to keep backwards compatibility in type checking, so I'll give up on that. If anyone has an idea, feel free to push directly to my branch (or to post suggestions if you don't have the commit bit).

max-sixty · 2024-04-14T17:39:56Z

However, since the copy kwarg is new in numpy>=2, mypy will fail to type check properly for versions of numpy<2. I have no idea how to keep backwards compatibility in type checking, so I'll give up on that.

Definitely the correct call imo!

But can we ensure the mypy env has the latest version so that it passes in CI?

keewis · 2024-04-21T18:03:04Z

But can we ensure the mypy env has the latest version so that it passes in CI?

The issue here is that this is the upcoming numpy 2.0 release, of which at the moment there is only a release candidate. So while upgrading the 2 mypy CIs might work, someone will have to check the upstream-dev mypy CI to see if there's anything to fix (there definitely is, though it would probably be best to wait with that until we figured out what to do with the runtime errors).

In general, though, I think we need to figure out what to do with backwards compatibility in typing.

max-sixty · 2024-04-21T18:29:46Z

Would it be reasonable to lock the mypy tests at numpy <2, and then when numpy>=2 is released, we do a single PR which upgrades our mypy tests to numpy>=2? And then we can merge this PR then?

I think trying to fix mypy for multiple dependency versions might be too hard (I already find it difficult for a single version!)

keewis · 2024-04-21T20:22:31Z

probably. However, I don't know enough about typing to get that to work. So if anyone has ideas, feel free to push to this PR.

max-sixty · 2024-04-21T21:06:28Z

(OK — just tbc — my suggestion is not to change the typing. But instead to lock the CI dependencies for mypy at numpy <2 until numpy 2 is released, at which point we lock to numpy > 2. And then to merge anything that requires numpy 2 typing, which may include this PR...)

keewis · 2024-04-21T21:18:49Z

you mean, let's try reverting the typing and see if everything passes? I remember changing the typing because the CI failed, so as far as I can tell (with my limited knowledge of typing) at least some changes are necessary.

max-sixty · 2024-04-21T21:42:08Z

I'm saying that if the typing is proving difficult, we should abandon the goal of mypy passing with both numpy 1 and numpy 2 as dependencies.

For tests, we should really try to pass with both numpy 1 & 2, because we can easily have if/else branches, and it affects people running the code.

But with typing it's both more difficult and less useful...

keewis · 2024-04-21T21:45:40Z

makes sense. What I'm saying, though, is that without any changes at all to the typing, mypy won't pass either. So we might have to find a way to be runtime-compatible with numpy>=2 without attempting to be typing-compatible (but how to do that I don't know).

max-sixty · 2024-04-22T04:28:34Z

find a way to be runtime-compatible with numpy>=2 without attempting to be typing-compatible (but how to do that I don't know).

Either our trusty type: ignore or to wait for numpy 2...?

I don't know enough about typing to ignore typing issues within a protocol, so that was the only way I could figure out how to get this PR unstuck. Once we know what to do here, we can remove the ignore in a new PR.

keewis · 2024-04-30T12:16:47Z

Since I really have no idea how to fix the typing issues (ignore comments won't work because we usually don't call the protocol ourselves), I've decided to ignore the warning in all our CI for now, and punt the change to the signature in most cases to a future PR. Hopefully this allows us to move forward with this PR.

xarray/core/dataset.py

keewis · 2024-05-01T13:43:02Z

looks like this works now? Should we merge?

dcherian · 2024-05-01T15:34:46Z

Thanks for working through this @keewis Can you open an issue please?

* origin/main: call `np.cross` with 3D vectors only (#8993) Mark `test_use_cftime_false_standard_calendar_in_range` as an expected failure (#8996) Migration of datatree/ops.py -> datatree_ops.py (#8976) avoid a couple of warnings in `polyfit` (#8939)

keewis added 2 commits April 13, 2024 12:55

use numpy.finfo instead of numpy.core.finfo

c7df27b

add copy to the signature of __array__

88c3670

keewis added the run-upstream Run upstream CI label Apr 13, 2024

dcherian and others added 2 commits April 13, 2024 08:45

Merge branch 'main' into polyfit-warnings

7127f3c

don't pass copy to asarray

e77463d

The `copy` kwarg appears to only exist in `numpy>=2`.

keewis commented Apr 14, 2024

View reviewed changes

try making the typing the same for all overloads

441482c

keewis added 2 commits April 14, 2024 19:02

change the typing for NamedArray.__array__

4ad4000

fix the signature of __array__

d830cef

Merge branch 'main' into polyfit-warnings

4f0ab1f

keewis mentioned this pull request Apr 29, 2024

more engine environment tricks in preparation for numpy>=2 #8978

Merged

1 task

keewis and others added 5 commits April 29, 2024 17:14

Merge branch 'main' into polyfit-warnings

687485f

Merge branch 'main' into polyfit-warnings

4ea799f

revert the change to the signature of __array__

bb73a31

ignore all deprecation warnings about __array__ gaining a new kwarg

d65c1e5

I don't know enough about typing to ignore typing issues within a protocol, so that was the only way I could figure out how to get this PR unstuck. Once we know what to do here, we can remove the ignore in a new PR.

add back the copy kwarg if the protocol does not have type hints

7b0b077

dcherian reviewed Apr 30, 2024

View reviewed changes

xarray/core/dataset.py Outdated Show resolved Hide resolved

Update xarray/core/dataset.py

709f338

Merge branch 'main' into polyfit-warnings

92d3910

dcherian merged commit 748bb3a into pydata:main May 1, 2024
26 of 28 checks passed

keewis deleted the polyfit-warnings branch May 1, 2024 16:42

jni mentioned this pull request Jun 18, 2024

Remove NumPy 2 pin, fix direct colormap with keys outside data dtype napari/napari#6998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid a couple of warnings in `polyfit` #8939

avoid a couple of warnings in `polyfit` #8939

keewis commented Apr 13, 2024 •

edited

Loading

keewis Apr 14, 2024 •

edited

Loading

Illviljan Apr 14, 2024

Illviljan Apr 14, 2024 •

edited

Loading

keewis commented Apr 14, 2024

keewis commented Apr 14, 2024 •

edited

Loading

max-sixty commented Apr 14, 2024 •

edited

Loading

keewis commented Apr 21, 2024

max-sixty commented Apr 21, 2024 •

edited

Loading

keewis commented Apr 21, 2024 •

edited

Loading

max-sixty commented Apr 21, 2024

keewis commented Apr 21, 2024

max-sixty commented Apr 21, 2024

keewis commented Apr 21, 2024 •

edited

Loading

max-sixty commented Apr 22, 2024

keewis commented Apr 30, 2024

keewis commented May 1, 2024

dcherian commented May 1, 2024

avoid a couple of warnings in polyfit #8939

avoid a couple of warnings in polyfit #8939

Conversation

keewis commented Apr 13, 2024 • edited Loading

keewis Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

Illviljan Apr 14, 2024

Choose a reason for hiding this comment

Illviljan Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

keewis commented Apr 14, 2024

keewis commented Apr 14, 2024 • edited Loading

max-sixty commented Apr 14, 2024 • edited Loading

keewis commented Apr 21, 2024

max-sixty commented Apr 21, 2024 • edited Loading

keewis commented Apr 21, 2024 • edited Loading

max-sixty commented Apr 21, 2024

keewis commented Apr 21, 2024

max-sixty commented Apr 21, 2024

keewis commented Apr 21, 2024 • edited Loading

max-sixty commented Apr 22, 2024

keewis commented Apr 30, 2024

keewis commented May 1, 2024

dcherian commented May 1, 2024

avoid a couple of warnings in `polyfit` #8939

avoid a couple of warnings in `polyfit` #8939

keewis commented Apr 13, 2024 •

edited

Loading

keewis Apr 14, 2024 •

edited

Loading

Illviljan Apr 14, 2024 •

edited

Loading

keewis commented Apr 14, 2024 •

edited

Loading

max-sixty commented Apr 14, 2024 •

edited

Loading

max-sixty commented Apr 21, 2024 •

edited

Loading

keewis commented Apr 21, 2024 •

edited

Loading

keewis commented Apr 21, 2024 •

edited

Loading