-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
ENH: Add dtype support to the array comparison ops #18128
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
Conversation
# `Sequence[int]`) and `str`. As `str` is a recusive sequence of | ||
# strings, it will pass through the final overload otherwise | ||
|
||
@overload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize the various overloads:
- Overload for filtering out any
str
/bytes
-based array-likes. This is needed asbytes
would otherwise be recognized as aSequence[int]
sub-type (which is bad) andstr
would otherwise be caught by overload 6. (asstr
is a sequence of strings, which is a sequence of strings, etc.). - Number-based overload
- Timedelta-based overload
- Datetime-based overload
- Object-based overload.
other
is typed asAny
here as object arrays can be extremely flexible (depending on the actual underlying objects). - A final overload to handle all sequences whose nesting level is too deep for the previous overloads.
Just curious about something since I've been following the work to add typing support for dtypes: won't making a mutable container type covariant cause issues? For the usual reasons that, e.g. if |
@a-reich this is arguably more of a problem with the variancy of In any case, besides abovementioned issue the problem with leaving |
@BvB93 I am trying to get a full picture for this. For your last example, it seems clear that The |
@BvB93 I see what you mean about the abstract parts of the dtype hierarchy making it difficult. However, your point about invariance of |
To give a bit of background: _NBit_co = TypeVar(..., covariant=True)
class floating(inexact[_NBit_co]):
...
# Note that the types below are not necasrelly treated as `floating` subclasses,
# it's more along the line of how `Sequence[int]` is a subtype of plain `Sequence`.
#
# This greatly simplifies the rather complicated typing of precisions, as you can always
# just hit the panic button when things get too complicated: _e.g._ `return floating[Any]`
float16 = floating[_16Bit]
float32 = floating[_32Bit]
float64 = floating[_64Bit] The key point is that because I recall from #17540 that something like this was actually possible for invariant parameters, but I |
|
Never mind #18128 (comment), turns out there were, unfortunately, detrimental problems with the therein referenced PR |
Needs rebase |
The dtypes scalar-type and ndarrays' dtype are now covariant instead of invariant. This change is necasary in order to ensure that all generic subclasses can be used as underlying scalar type.
…ere neglected More specifically operations between array-likes of `timedelta64` and `ndarray`s that can be cast into `timedelta64`. For example: ar_i = np.array([1]) seq_m = [np.timedelta64()] ar_i > seq_m
The branch has been rebased and the tests seem to be passing. |
Thanks Bas. |
Per the title: this PR adds dtype support to the array comparison ops (
__lt__
,__le__
, etc.).Note that in order to get this to work the variance of the
dtype
andndarray
parameters had to be changed invariant (the default) to covariant. Without this, the likes ofnp.dtype[np.int64]
would not be considered sub-types ofnp.dtype[np.integer]
, making things needlessly complicated.Examples