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

ENH: Add annotations for comparison operations #17778

Merged
merged 2 commits into from
Nov 29, 2020
Merged

Conversation

BvB93
Copy link
Member

@BvB93 BvB93 commented Nov 15, 2020

This pr adds annotations for the comparison operations of ndarray / generic:

  • __gt__
  • __ge__
  • __lt__
  • __le__

The equivalency operators where deliberately excluded as it is currently impossible to properly type
them due to the existence of object.__eq__. See #17368 (comment) for more details.

@overload
def __call__(self, __other: _T) -> bool_: ...
@overload
def __call__(self, __other: ArrayLike) -> Union[ndarray, bool_]: ...
Copy link
Member Author

@BvB93 BvB93 Nov 15, 2020

Choose a reason for hiding this comment

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

The comparison ops of generic are one the few that accept actual array-like object (rather than just arrays and/or scalar). Consequently, the signatures are still a bit too broad whenever array-like objects are involved (something to revisit in a future PR after #17719):

import numpy as np

U = np.str_()
i8 = np.int64()

i8 > U  # This is considered acceptable (for now) due to the broad definition of `ArrayLike`

@BvB93 BvB93 modified the milestone: 1.20.0 release Nov 16, 2020
@@ -312,6 +314,12 @@ def __call__(
class _NumberOp(Protocol):
def __call__(self, __other: _NumberLike) -> number: ...

class _ComparisonOp(Protocol[_T]):
@overload
def __call__(self, __other: _T) -> bool_: ...
Copy link
Member

Choose a reason for hiding this comment

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

Are these overloads in the right order? I can't find docs so quickly, but I had expected overloads to be checked in order, so _T would also match ArrayLike.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in practice the first overload is used for all valid scalar-types, which always return bool_, while the second one is used for the much broader ArrayLike (a superset of the first overload).

Copy link
Member Author

Choose a reason for hiding this comment

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

Also note that _T is a typevar, so it should be substituted with an actual value.
For example, _ComparisonOp[datetime64] replaces _T with datetime64.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in practice the first overload is used for all valid scalar-types

Thanks. That's what I was (am) fuzzy about. The Python docs on TypeVar are pretty bad, and PEP 484 says "By default, a type variable ranges over all possible types.". My interpretation was really "all", not "all scalar". You can use TypeVar( ...bound= with classes, mappings, etc. as bounds. So I thought _T would match array-likes as well here.

Copy link
Member Author

@BvB93 BvB93 Nov 28, 2020

Choose a reason for hiding this comment

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

For an unbound (i.e. unassigned) typevars this statement is actually true, even if the bound parameter is specified (because reasons...). This is the very reason why it is recommended to always assign some value to subscriptable types, e.g. use Sequence[Any] instead of plain Sequence.

The above-mentioned fortunately becomes irrelevant once a value has been assigned to the typevar (Seq3 in the example below).

from typing import TypeVar, Sequence

T = TypeVar("T", bound=int)

Seq1 = Sequence  # equivalent to `Sequence[Any]`
Seq2 = Sequence[T]  # equivalent to `Sequence[Any]`
Seq3 = Seq2[int]  # equivalent to `Sequence[int]`

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that makes more sense now.

@rgommers rgommers merged commit 1e4c12d into numpy:master Nov 29, 2020
@rgommers
Copy link
Member

In it goes, thanks @BvB93

@rgommers rgommers added this to the 1.21.0 release milestone Nov 29, 2020
@BvB93 BvB93 deleted the comparison branch November 29, 2020 14:07
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.

None yet

2 participants