-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
Fix Series v Index bool ops #22173
Fix Series v Index bool ops #22173
Changes from 5 commits
b0ab1a4
d2d1564
69fc8d9
f043342
924ecde
58cd628
5f48482
bb54b5e
523eb04
17668a0
44aa551
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1525,23 +1525,22 @@ def _bool_method_SERIES(cls, op, special): | |
Wrapper function for Series arithmetic operations, to avoid | ||
code duplication. | ||
""" | ||
op_name = _get_op_name(op, special) | ||
|
||
def na_op(x, y): | ||
try: | ||
result = op(x, y) | ||
except TypeError: | ||
if isinstance(y, list): | ||
y = construct_1d_object_array_from_listlike(y) | ||
|
||
if isinstance(y, (np.ndarray, ABCSeries)): | ||
if (is_bool_dtype(x.dtype) and is_bool_dtype(y.dtype)): | ||
result = op(x, y) # when would this be hit? | ||
else: | ||
x = ensure_object(x) | ||
y = ensure_object(y) | ||
result = libops.vec_binop(x, y, op) | ||
assert not isinstance(y, (list, ABCSeries, ABCIndexClass)) | ||
if isinstance(y, np.ndarray): | ||
# bool-bool dtype operations should be OK, should not get here | ||
assert not (is_bool_dtype(x) and is_bool_dtype(y)) | ||
x = ensure_object(x) | ||
y = ensure_object(y) | ||
result = libops.vec_binop(x, y, op) | ||
else: | ||
# let null fall thru | ||
assert lib.is_scalar(y) | ||
if not isna(y): | ||
y = bool(y) | ||
try: | ||
|
@@ -1561,33 +1560,40 @@ def wrapper(self, other): | |
is_self_int_dtype = is_integer_dtype(self.dtype) | ||
|
||
self, other = _align_method_SERIES(self, other, align_asobject=True) | ||
res_name = get_op_result_name(self, other) | ||
|
||
if isinstance(other, ABCDataFrame): | ||
# Defer to DataFrame implementation; fail early | ||
return NotImplemented | ||
|
||
elif isinstance(other, ABCSeries): | ||
name = get_op_result_name(self, other) | ||
elif isinstance(other, (ABCSeries, ABCIndexClass)): | ||
is_other_int_dtype = is_integer_dtype(other.dtype) | ||
other = fill_int(other) if is_other_int_dtype else fill_bool(other) | ||
|
||
filler = (fill_int if is_self_int_dtype and is_other_int_dtype | ||
else fill_bool) | ||
|
||
res_values = na_op(self.values, other.values) | ||
unfilled = self._constructor(res_values, | ||
index=self.index, name=name) | ||
return filler(unfilled) | ||
ovalues = other.values | ||
finalizer = lambda x: x | ||
|
||
else: | ||
# scalars, list, tuple, np.array | ||
filler = (fill_int if is_self_int_dtype and | ||
is_integer_dtype(np.asarray(other)) else fill_bool) | ||
|
||
res_values = na_op(self.values, other) | ||
unfilled = self._constructor(res_values, index=self.index) | ||
return filler(unfilled).__finalize__(self) | ||
is_other_int_dtype = is_integer_dtype(np.asarray(other)) | ||
if is_list_like(other) and not isinstance(other, np.ndarray): | ||
# TODO: Can we do this before the is_integer_dtype check? | ||
# could the is_integer_dtype check be checking the wrong | ||
# thing? e.g. other = [[0, 1], [2, 3], [4, 5]]? | ||
other = construct_1d_object_array_from_listlike(other) | ||
|
||
ovalues = other | ||
finalizer = lambda x: x.__finalize__(self) | ||
|
||
filler = (fill_int if is_self_int_dtype and is_other_int_dtype | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comment on what is going on with the filler |
||
else fill_bool) | ||
res_values = na_op(self.values, ovalues) | ||
unfilled = self._constructor(res_values, | ||
index=self.index, name=res_name) | ||
filled = filler(unfilled) | ||
return finalizer(filled) | ||
|
||
wrapper.__name__ = op_name | ||
return wrapper | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -603,6 +603,20 @@ def test_ops_datetimelike_align(self): | |
result = (dt2.to_frame() - dt.to_frame())[0] | ||
assert_series_equal(result, expected) | ||
|
||
def test_bool_ops_with_index(self): | ||
# GH#19792 | ||
# TODO: reversed ops still raises, GH#22092 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have an (unless you plan to patch the reversed op in the near term, then not needed) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on this idea. I'll wait til AM to implement it, ideally on the follow-up to #22153. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add this test? |
||
ser = Series([True, False, True]) | ||
idx = pd.Index([False, True, True]) | ||
|
||
result = ser & idx | ||
expected = Series([False, False, True]) | ||
assert_series_equal(result, expected) | ||
|
||
result = ser | idx | ||
expected = Series([True, True, True]) | ||
assert_series_equal(result, expected) | ||
|
||
def test_operators_bitwise(self): | ||
# GH 9016: support bitwise op for integer types | ||
index = list('bca') | ||
|
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.
From an error reporting perspective, I might go for:
A bare assert statement would unfortunately not provide much info to the end-user (admittedly, my proposed change assumes that the
TypeError
raised has some kind of message). At the very least, adding an error message on theassert
would be useful.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.
This comment BTW applies to any of your other
assert
statements.