-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
TYP: mostly Hashtable and ArrowExtensionArray #56689
Conversation
def any(self, *, skipna: bool, **kwargs) -> bool | NAType: | ||
... | ||
|
||
def any(self, *, skipna: bool = True, **kwargs) -> bool | NAType: |
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.
Is this deviation from the base class on purpose (base class returns always a bool)?
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.
I think so. This can return NA with skipna=False
In [5]: pd.Series([pd.NA], dtype="bool[pyarrow]").any(skipna=False)
Out[5]: <NA>
In [6]: pd.Series([pd.NA], dtype="boolean").any(skipna=False)
Out[6]: <NA>
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.
Okay - I will adjust the annotations in the base class to reflect that
@@ -1541,25 +1541,24 @@ def construct_1d_arraylike_from_scalar( | |||
if isinstance(dtype, ExtensionDtype): | |||
cls = dtype.construct_array_type() | |||
seq = [] if length == 0 else [value] | |||
subarr = cls._from_sequence(seq, dtype=dtype).repeat(length) | |||
return cls._from_sequence(seq, dtype=dtype).repeat(length) |
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.
Avoids a mypy error (assigning a new type to subarr).
@@ -1311,18 +1311,18 @@ def map(self, mapper, na_action=None): | |||
@overload | |||
def any( | |||
self, *, skipna: Literal[True] = ..., axis: AxisInt | None = ..., **kwargs | |||
) -> bool: | |||
) -> np.bool_: |
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.
The pandas code expects it to return np.bool_ and not bool (see failing CI on last commit)!
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.
I think it would be better longer term if this returned bool
. Would that require a lot of change?
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.
It is easy to fix inside all/any but there are at least two places (maybe more) that error after that change:
pandas/core/arrays/masked.py:1164
pandas/core/arrays/masked.py:840
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.
OK this can be a follow up
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.
All looks good to me - will let others review/approve/merge
Thanks @twoertwein |
No description provided.