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

Add Ellipsis typehints #7017

Merged
merged 2 commits into from
Sep 11, 2022
Merged

Add Ellipsis typehints #7017

merged 2 commits into from
Sep 11, 2022

Conversation

headtr1ck
Copy link
Collaborator

This PR adds an Ellipsis typehint to some functions.

Interestingly mypy did not complain at the tests before, I assume it is because "..." is Hashable or something like that?

I don't know what to do with reductions, since they also support ellipsis, but it is basically the same as using None.
Therefore, I assume it is not necessary to expose this feature.

Did I miss any functions where ellipsis is supported? It is hard to look for "..."... xD


T_Dataset = TypeVar("T_Dataset", bound="Dataset")
T_DataArray = TypeVar("T_DataArray", bound="DataArray")
T_Variable = TypeVar("T_Variable", bound="Variable")
T_Array = TypeVar("T_Array", bound="AbstractArray")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops that was not supposed to be checked in, but I guess it doesn't hurt.

@@ -3769,7 +3770,7 @@ def imag(self: T_DataArray) -> T_DataArray:
def dot(
self: T_DataArray,
other: T_DataArray,
dims: Hashable | Sequence[Hashable] | None = None,
dims: str | Iterable[Hashable] | Ellipsis | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent! Great that we can roll the str | Iterable[Hashable] out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, as soon as you try to type some internals you will run into problems with Hashable | Sequence[Hashable].
This really seems to be the only way.

We should probably add a parser for this in utils such that all methods handle it the same.
Maybe I can come up with a PR for that.

Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Thanks! Another good one!

Do you know whether Ellipsis works now? I had put an issue in with mypy, and discussed it briefly (with Guido!), is it now in mypy? (and regardless, this will be nicely forward-compatible)

@headtr1ck
Copy link
Collaborator Author

Do you know whether Ellipsis works now? I had put an issue in with mypy, and discussed it briefly (with Guido!), is it now in mypy? (and regardless, this will be nicely forward-compatible)

I saw that issue :)
That was the only way I could get it to work, in runtime it still does not seem to work. Maybe "Ellipsis" (with "") could work?

@max-sixty
Copy link
Collaborator

That makes sense. Until mypy make the change, I think this is a nice signpost for users, and will be easily forward compatible…

@max-sixty max-sixty merged commit b018442 into pydata:main Sep 11, 2022
@dcherian
Copy link
Contributor

I don't know what to do with reductions, since they also support ellipsis, but it is basically the same as using None.

For Groupby reductions ... means reduce over all dimensions; None means reduce over groups's dimensions

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.

3 participants