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

improve typing of DataArray and Dataset reductions #6746

Merged
merged 9 commits into from
Sep 14, 2023

Conversation

rhkleijn
Copy link
Contributor

@rhkleijn rhkleijn commented Jul 2, 2022

This PR makes the typing of reduction methods (count, all, any, max, min, mean, prod, sum, std, var, median) for Dataset and DataArray consistent with the typing of most other methods of Dataset and DataArray after the great recent improvements by @headtr1ck in e.g. #6661.

@rhkleijn rhkleijn marked this pull request as draft July 2, 2022 21:22
@headtr1ck
Copy link
Collaborator

Noble effort, but I think we will have to wait until python/mypy#12846 is solved, otherwise users will get false positives...

@rhkleijn
Copy link
Contributor Author

rhkleijn commented Jul 3, 2022

Agreed, I just reached the same conclusion.

@headtr1ck
Copy link
Collaborator

headtr1ck commented Aug 13, 2023

Since python/mypy#12846 seems to have been fixed, we could give this another go.

Edit: Actually, now that the Self type should be fully supported, we could just move to this directly!

@rhkleijn rhkleijn force-pushed the reductions-typevar branch from defb134 to 2deffe9 Compare August 18, 2023 00:33
@rhkleijn
Copy link
Contributor Author

rhkleijn commented Aug 18, 2023

Rebasing my branch required solving quite some merge conflicts, which was to be expected as this PR had run more than 400 commits behind. I adapted the PR to now use Self as you suggested.
It took a few iterations to satisfy both mypy 1.4.1 (in my development enviroment) and mypy 1.5.1 (in CI) at the same time, but now CI is fully green.

@rhkleijn rhkleijn marked this pull request as ready for review August 18, 2023 01:06
@dcherian dcherian requested a review from headtr1ck August 21, 2023 19:44
Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

Thanks for updating this old PR.
Couple minor remarks.

Also, feel free to add an entry to what's new!

xarray/core/computation.py Outdated Show resolved Hide resolved
xarray/core/dataset.py Outdated Show resolved Hide resolved
@headtr1ck headtr1ck added plan to merge Final call for comments topic-typing labels Sep 13, 2023
@headtr1ck headtr1ck enabled auto-merge (squash) September 13, 2023 19:24
@headtr1ck headtr1ck merged commit 218ea21 into pydata:main Sep 14, 2023
29 checks passed
@headtr1ck
Copy link
Collaborator

Thanks for this PR!

@rhkleijn
Copy link
Contributor Author

Thank you for taking care of the last few steps yourself and merging this PR! I just came back from vacation and just realized it would have been nicer if I had mentioned that I was going to be unresponsive for several weeks.

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

Successfully merging this pull request may close these issues.

2 participants