Fix Series v Index bool ops #22173
Fix Series v Index bool ops #22173
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22173 +/- ##
==========================================
+ Coverage 92.17% 92.18% +<.01%
==========================================
Files 169 169
Lines 50778 50783 +5
==========================================
+ Hits 46807 46814 +7
+ Misses 3971 3969 -2
Continue to review full report at Codecov.
|
gfyoung
added
Bug
Algos
| x = ensure_object(x) | ||
| y = ensure_object(y) | ||
| result = libops.vec_binop(x, y, op) | ||
| assert not isinstance(y, (list, ABCSeries, ABCIndexClass)) |
There was a problem hiding this comment.
From an error reporting perspective, I might go for:
if isinstance(y, (list, ABCSeries, ABCIndexClass)):
raiseA 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 the assert would be useful.
There was a problem hiding this comment.
This comment BTW applies to any of your other assert statements.
| @@ -1433,6 +1433,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.
Can we have an xfail test for that in this PR?
(unless you plan to patch the reversed op in the near term, then not needed)
There was a problem hiding this comment.
+1 on this idea. I'll wait til AM to implement it, ideally on the follow-up to #22153.
|
let me have another look, can you rebase |
|
Rebased+green |
pandas/core/ops.py
Outdated
| 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.
can you add a comment on what is going on with the filler
| @@ -1433,6 +1433,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 | |||
|
Hello @jbrockmendel! Thanks for updating the PR.
|
|
doesn't this overlap with #22293 ? |
|
Yep, commented to that effect over there. |
|
ok then, merge that one first? |
Sure. After rebase this'll be a small follow-up |
| @@ -627,6 +604,42 @@ def test_ops_datetimelike_align(self): | |||
| result = (dt2.to_frame() - dt.to_frame())[0] | |||
| assert_series_equal(result, expected) | |||
|
|
|||
| @pytest.mark.parametrize('op', [ | |||
| operator.and_, | |||
There was a problem hiding this comment.
pls add these as a fixture in conftest (follow-on PR)
Series boolean ops tests need some serious cleanup and fleshing out. This gets separates out one fix before going in to clean up the tests.