Simplify bool ops closures, improve tests #19795

jbrockmendel · 2018-02-20T18:59:45Z

Series bool ops are not going to be possible to dispatch to their Index counterparts, so this one I'm mostly going to leave alone. This breaks up the tests, adds an xfail for op(Series, Categorical), and fixes+tests op(series, Index). It makes some headway in simplifying the closure so the possible paths through the code are more obvious.

There is a TODO comment on line 1113 where I think there is a potential bug, but need someone who understands the intended behavior better to weigh in.

jreback

how about we do the tests with appropriate xfails separately. it is really confusing what you are actually changing.

jreback · 2018-02-21T00:17:24Z

doc/source/whatsnew/v0.23.0.txt

@@ -866,3 +866,4 @@ Other
 ^^^^^

 - Improved error message when attempting to use a Python keyword as an identifier in a ``numexpr`` backed query (:issue:`18221`)
+- Boolean operations ``&, |, ^`` between a ``Series`` and an ``Index`` will no longer raise ``TypeError`` (:issue:`19792`)


this is not a bug, rather an API breaking change.

jreback · 2018-02-21T00:18:39Z

pandas/core/ops.py

-                    x = _ensure_object(x)
-                    y = _ensure_object(y)
-                    result = lib.vec_binop(x, y, op)
+            assert not isinstance(y, (list, ABCSeries, pd.Index))


this is oddly specific. what about a tuple?

basicaly you are trying to catch ndarray or non-list-like right?

This assertion is codifying existing behavior. ATM the line following this checks for isinstance(y, (np.ndarray, ABCSeries)) and the line above converts lists to ndarray. I removed the ABCSeries part, converted lists a step earlier, and added the assertion to make sure we don't accidentally miss either of these cases.

remove this and let's instead qualify the below with

if isinstance(y, np.ndarray): ... elif is_list_like(.....): else: # scalar

jreback · 2018-02-21T00:19:02Z

pandas/core/ops.py

-                    result = lib.vec_binop(x, y, op)
+            assert not isinstance(y, (list, ABCSeries, pd.Index))
+            if isinstance(y, np.ndarray):
+                # This next assertion is here because there used to be


this is not a useful comment. imagine a new reader coming along with no history

Yah, not sure what to put here. ATM there is a non-reachable branch with a TODO comment specifically asking how it would be reached. This seemed worth documenting somehow since someone at some point found it worth special-casing.

i think the assertion pretty much explains this, though maybe add some text that we are trying to compare non-bool things here

jreback · 2018-02-21T00:19:39Z

pandas/core/ops.py

-            filler = (fill_int if is_self_int_dtype and
-                      is_integer_dtype(np.asarray(other)) else fill_bool)
+            is_other_int_dtype = is_integer_dtype(np.asarray(other))
+            if isinstance(other, list):


a list only?

ATM lists are handled inside na_op on 1066-1067. This is just moving the same two lines up the call stack (and outside of an exception-handling block)

the comment also includes tuple and np.array, should not this check for also for tuple?

I guess it depends on if we want to treat tuples as "an array the user forgot to wrap in an array". For lists we do that pretty consistently (e.g. on line 1034 and 1172 in core.ops), but not so much for tuples. I don't have a strong opinion as to what the ideal behavior is, but would prefer to address it separately from this cleanup/testing PR. (it could go in the same Issue as the TODO comment just below this isinstance check)

this should be is_list_like

jbrockmendel · 2018-02-21T01:07:58Z

how about we do the tests with appropriate xfails separately. it is really confusing what you are actually changing.

OK

codecov · 2018-02-21T01:21:37Z

Codecov Report

Merging #19795 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #19795      +/-   ##
==========================================
- Coverage   91.61%    91.6%   -0.01%     
==========================================
  Files         150      150              
  Lines       48887    48891       +4     
==========================================
+ Hits        44786    44789       +3     
- Misses       4101     4102       +1

Flag	Coverage Δ
#multiple	`89.98% <100%> (-0.01%)`	⬇️
#single	`41.8% <65.38%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/ops.py	`96.93% <100%> (+0.18%)`	⬆️
pandas/util/testing.py	`83.64% <0%> (-0.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 740ad9a...c03cc55. Read the comment docs.

codecov · 2018-02-21T01:21:50Z

Codecov Report

Merging #19795 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #19795      +/-   ##
==========================================
+ Coverage   91.66%   91.66%   +<.01%     
==========================================
  Files         150      150              
  Lines       49026    49031       +5     
==========================================
+ Hits        44939    44945       +6     
+ Misses       4087     4086       -1

Flag	Coverage Δ
#multiple	`90.04% <100%> (ø)`	⬆️
#single	`41.83% <65.21%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/ops.py	`97.11% <100%> (+0.17%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9958ce6...0d99c94. Read the comment docs.

…ol_ops3

jreback · 2018-02-22T00:03:18Z

needs a rebase and I have yet to actually review

…ol_ops3

jreback · 2018-02-22T11:36:04Z

pandas/core/ops.py

-                    result = lib.vec_binop(x, y, op)
+            assert not isinstance(y, (list, ABCSeries, pd.Index))
+            if isinstance(y, np.ndarray):
+                # This next assertion is here because there used to be


i think the assertion pretty much explains this, though maybe add some text that we are trying to compare non-bool things here

jreback · 2018-02-22T11:37:21Z

pandas/core/ops.py

-            filler = (fill_int if is_self_int_dtype and
-                      is_integer_dtype(np.asarray(other)) else fill_bool)
+            is_other_int_dtype = is_integer_dtype(np.asarray(other))
+            if isinstance(other, list):


the comment also includes tuple and np.array, should not this check for also for tuple?

jreback · 2018-02-23T11:19:06Z

pandas/core/ops.py

-                    x = _ensure_object(x)
-                    y = _ensure_object(y)
-                    result = lib.vec_binop(x, y, op)
+            assert not isinstance(y, (list, ABCSeries, pd.Index))


remove this and let's instead qualify the below with

if isinstance(y, np.ndarray): ... elif is_list_like(.....): else: # scalar

jreback · 2018-02-23T11:19:44Z

pandas/core/ops.py

-            filler = (fill_int if is_self_int_dtype and
-                      is_integer_dtype(np.asarray(other)) else fill_bool)
+            is_other_int_dtype = is_integer_dtype(np.asarray(other))
+            if isinstance(other, list):


this should be is_list_like

…ol_ops3

jbrockmendel · 2018-02-24T19:16:55Z

Reverted some suggested changes because I'm not confident they handle Categorical correctly. Until the tests+docs for these methods are fleshed out, sticking to cleanup without changing behavior.

…ol_ops3

jreback · 2018-03-01T11:27:18Z

needs a rebase on master. note always rebase when pushing.

…ol_ops3

jreback · 2018-03-02T11:22:22Z

pandas/core/ops.py

-                    x = _ensure_object(x)
-                    y = _ensure_object(y)
-                    result = libops.vec_binop(x, y, op)
+            assert not isinstance(y, (list, ABCSeries, pd.Index))


add tuple here, or assert is_list_like and not np.ndarray

or maybe just remove the assertion and make the else into an is_scalar and make an else that raises an AssertionError

As mentioned above, I’m not inclined to change any behavior until the docs/tests are fleshed out.

then let's do that first (in this PR or a pre-cursor)

jreback · 2018-03-02T11:25:01Z

pandas/core/ops.py

-            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):


then thismust be a scalar, put this in another elsif rather than trying to catch the list-like case (ndarray, list, tuple) AND scalar case here

jreback · 2018-03-02T11:26:15Z

pandas/tests/series/test_operators.py

-        s_fff = Series([False, False, False], index=index)
-        s_tff = Series([True, False, False], index=index)
-        s_empty = Series([])
+        # We cannot wrap s111.astype(np.int32) with pd.Index because it


can you exercise both lhs and rhs are scalars (with a Series/Index) (True, False) and also None/np.nan

i c you might have these cases below, if you do make add _scalars in the test name (and see if you can fully exercise them)

jreback · 2018-03-02T11:27:35Z

there is likely an issue for this already, see if you can find it

jreback · 2018-03-04T20:18:52Z

pandas/core/ops.py

-                    x = _ensure_object(x)
-                    y = _ensure_object(y)
-                    result = libops.vec_binop(x, y, op)
+            assert not isinstance(y, (list, ABCSeries, pd.Index))


then let's do that first (in this PR or a pre-cursor)

jbrockmendel added 3 commits February 20, 2018 09:02

split bool ops test, parametrize where possible

619d2bb

remove non-standard imports of nan, inf

7cf9a7a

simplify bool ops closures

f5b262b

jreback requested changes Feb 21, 2018

View reviewed changes

jreback added the Dtype Conversions Unexpected or buggy dtype conversions label Feb 21, 2018

move whatsnew to breaking changes, revert non-central test diff

c03cc55

jbrockmendel added 2 commits February 20, 2018 17:29

Merge branch 'master' of https://github.com/pandas-dev/pandas into bo…

235f969

…ol_ops3

fixup duplicate line

9aa41ec

Merge branch 'master' of https://github.com/pandas-dev/pandas into bo…

e721b64

…ol_ops3

jreback requested changes Feb 22, 2018

View reviewed changes

reword comment

a591078

jreback requested changes Feb 23, 2018

View reviewed changes

jbrockmendel added 2 commits February 23, 2018 13:56

Merge branch 'master' of https://github.com/pandas-dev/pandas into bo…

8243ec8

…ol_ops3

use is_list_like

f19a596

Merge branch 'master' of https://github.com/pandas-dev/pandas into bo…

f9ff20b

…ol_ops3

jbrockmendel force-pushed the bool_ops3 branch from 93d46c2 to f9ff20b Compare March 1, 2018 02:54

Merge branch 'master' of https://github.com/pandas-dev/pandas into bo…

0d99c94

…ol_ops3

jreback requested changes Mar 2, 2018

View reviewed changes

jreback added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Mar 2, 2018

jreback requested changes Mar 4, 2018

View reviewed changes

jbrockmendel closed this Mar 4, 2018

jbrockmendel mentioned this pull request May 21, 2018

ENH: Support operators for ExtensionArray #20889

Closed

4 tasks

jbrockmendel deleted the bool_ops3 branch April 5, 2020 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify bool ops closures, improve tests #19795

Simplify bool ops closures, improve tests #19795

jbrockmendel commented Feb 20, 2018

jreback left a comment

jreback Feb 21, 2018

jbrockmendel Feb 21, 2018

jreback Feb 21, 2018

jbrockmendel Feb 21, 2018

jreback Feb 23, 2018

jreback Feb 21, 2018

jbrockmendel Feb 21, 2018

jreback Feb 22, 2018

jreback Feb 21, 2018

jbrockmendel Feb 21, 2018

jreback Feb 22, 2018

jbrockmendel Feb 22, 2018

jreback Feb 23, 2018

jbrockmendel commented Feb 21, 2018

codecov bot commented Feb 21, 2018

codecov bot commented Feb 21, 2018 •

edited

Loading

jreback commented Feb 22, 2018

jreback Feb 22, 2018

jreback Feb 22, 2018

jreback Feb 23, 2018

jreback Feb 23, 2018

jbrockmendel commented Feb 24, 2018

jreback commented Mar 1, 2018

jreback Mar 2, 2018

jreback Mar 2, 2018

jbrockmendel Mar 2, 2018

jreback Mar 4, 2018

jreback Mar 2, 2018

jreback Mar 2, 2018

jreback Mar 2, 2018

jreback commented Mar 2, 2018

jreback Mar 4, 2018

Simplify bool ops closures, improve tests #19795

Simplify bool ops closures, improve tests #19795

Conversation

jbrockmendel commented Feb 20, 2018

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 21, 2018

codecov bot commented Feb 21, 2018

Codecov Report

codecov bot commented Feb 21, 2018 • edited Loading

Codecov Report

jreback commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 24, 2018

jreback commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Mar 2, 2018

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2018 •

edited

Loading