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

Rewrite typed_ops #8204

Merged
merged 17 commits into from Sep 25, 2023
Merged

Rewrite typed_ops #8204

merged 17 commits into from Sep 25, 2023

Conversation

headtr1ck
Copy link
Collaborator

@headtr1ck headtr1ck commented Sep 18, 2023

@max-sixty
Copy link
Collaborator

(Looks great so far!)

@headtr1ck
Copy link
Collaborator Author

Basically all type: ignores in the new tests are currently broken.
Some numpy ones are expected because e.g. np.ndarray.__add__ is typed wrong and assumes that you get a np.ndarray if you do np.array() + xr.DataArray(), which is wrong. (See #7780)

Why some of the python type ops are failing I have to investigate.

@headtr1ck
Copy link
Collaborator Author

Wow, this turns out to be more tricky than I thought.
E.g. Variable(...) + DataArray(...) returns a Variable type. This is because np.typing.ArrayLike allows _SupportsArray, which both, variables and DataArrays are. Therefore in the typing of Variable.__add__, mypy thinks that DataArrays are allowed.

I really need a way to specify that we support ArrayLike but not DataArrays.....
Maybe we could hack it back like before and add an overload for DataArrays to return DataArray, which is not really correct but effectively resolves this problem.

@headtr1ck headtr1ck marked this pull request as ready for review September 24, 2023 18:37
@headtr1ck
Copy link
Collaborator Author

Now this PR brings the static typing of the ops methods to the best we can currently achieve.
Two mayor open points remain (including the one of the original issue) but they are upstream problems and we cannot fix those.

  1. Numpy: they type basically all their ops wrong. Whenever you do np.array() + xr.DataArray() you will get a np.ndarray according to mypy. The only workaround is to reverse the order, e.g. do xr.DataArray() + np.array(). Numpy needs to change their typing in a way that classes with __array_function__ take precedence (e.g. return NotImplemented/NoReturn) and classes with __array__ return np.ndarray.
  2. Typeshed: basic types like int or list pretend to accept anything, which is defacto not true. For non-compatible types they should return NotImplemented as well. See https://discuss.python.org/t/make-type-hints-for-eq-of-primitives-less-strict/34240

The added "unit" tests (basically only checks for mypy) have type ignores for these cases, so we will realize when something upstream has changed.

"invoking the `to_array()` method."
)
if TYPE_CHECKING:
# needed because __getattr__ is returning Any and otherwise
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the issue that is also causing this problem here: #4601

@headtr1ck headtr1ck added run-upstream Run upstream CI plan to merge Final call for comments labels Sep 24, 2023
@max-sixty
Copy link
Collaborator

Awesome job @headtr1ck ! Very impressive, thank you.

@kmuehlbauer kmuehlbauer mentioned this pull request Sep 24, 2023
5 tasks
@headtr1ck headtr1ck merged commit 565b23b into pydata:main Sep 25, 2023
30 of 34 checks passed
@headtr1ck headtr1ck deleted the typedops branch September 26, 2023 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants