-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix and test scalar extension dtype op corner case #22378
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
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.
Good catch :)
@@ -1228,8 +1228,8 @@ def wrapper(left, right): | |||
"{op}".format(typ=type(left).__name__, op=str_rep)) | |||
|
|||
elif (is_extension_array_dtype(left) or | |||
is_extension_array_dtype(right)): | |||
# TODO: should this include `not is_scalar(right)`? | |||
(is_extension_array_dtype(right) and not is_scalar(right))): |
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 checking that it is a string a bit more logical?
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 guess that would work. Elsewhere the same check is done with not is_scalar
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.
then ignore my comment and go for consistency
Codecov Report
@@ Coverage Diff @@
## master #22378 +/- ##
=======================================
Coverage 92.05% 92.05%
=======================================
Files 169 169
Lines 50709 50709
=======================================
Hits 46679 46679
Misses 4030 4030
Continue to review full report at Codecov.
|
@jorisvandenbossche think this is mergeable? I'm looking through the queue for stuff sufficiently small/easy to take off @jreback's plate. |
Yes, merged, thanks! |
Fixes the following behavior in master:
and the same for the reversed ops.