Bug: Logical operator of Series with Index (#22092) #22293

makbigc · 2018-08-12T14:15:20Z

closes Bool Ops: One bug hides another #22092
1 test added for &, | and ^ logical operator of series
whatsnew entry

gfyoung · 2018-08-13T07:04:11Z

doc/source/whatsnew/v0.24.0.txt

@@ -497,6 +497,6 @@ Other
 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now takes a ``text_color_threshold`` parameter to automatically lighten the text color based on the luminance of the background color. This improves readability with dark background colors without the need to limit the background colormap range. (:issue:`21258`)
 - Require at least 0.28.2 version of ``cython`` to support read-only memoryviews (:issue:`21688`)
 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now also supports tablewise application (in addition to rowwise and columnwise) with ``axis=None`` (:issue:`15204`)
-
+- Bug in the logical operator of :class:`Series` with :class:`Index` (:issue:`22092`) 


Perhaps something like:

Bug in logical operators handling :class:`Series` and :class:`Index` together (:issue:`22092`)

codecov · 2018-08-13T12:48:43Z

Codecov Report

Merging #22293 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #22293   +/-   ##
=======================================
  Coverage   92.16%   92.16%           
=======================================
  Files         169      169           
  Lines       50708    50708           
=======================================
  Hits        46734    46734           
  Misses       3974     3974

Flag	Coverage Δ
#multiple	`90.57% <100%> (ø)`	⬆️
#single	`42.35% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/ops.py	`96.94% <100%> (ø)`	⬆️

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 996f361...aaf8349. Read the comment docs.

jbrockmendel · 2018-08-13T17:35:43Z

doc/source/whatsnew/v0.24.0.txt

@@ -497,6 +497,6 @@ Other
 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now takes a ``text_color_threshold`` parameter to automatically lighten the text color based on the luminance of the background color. This improves readability with dark background colors without the need to limit the background colormap range. (:issue:`21258`)
 - Require at least 0.28.2 version of ``cython`` to support read-only memoryviews (:issue:`21688`)
 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now also supports tablewise application (in addition to rowwise and columnwise) with ``axis=None`` (:issue:`15204`)
-
+- Bug in the logical operators handling :class:`Series` and :class:`Index` together (:issue:`22092`) 


Describe the bug briefly. For an example, see #22173.

jbrockmendel · 2018-08-13T17:35:58Z

pandas/core/ops.py

@@ -1383,7 +1383,7 @@ def na_op(x, y):
            if isinstance(y, list):
                y = construct_1d_object_array_from_listlike(y)

-            if isinstance(y, (np.ndarray, ABCSeries)):
+            if isinstance(y, (np.ndarray, ABCSeries, ABCIndex)):


This should be ABCIndexClass, not ABCIndex.

jbrockmendel

Test looks good, some comments on the whatsnew and the edit in core.ops. I'm partial to #22173 for this, but not exactly unbiased there.

jreback · 2018-08-14T00:17:49Z

pandas/tests/series/test_operators.py

@@ -537,6 +537,23 @@ def test_comparison_flex_alignment_fill(self):
        exp = pd.Series([True, True, False, False], index=list('abcd'))
        assert_series_equal(left.gt(right, fill_value=0), exp)

+    def test_comparison_with_index(self):


can you check this on Int64Index as well. I think some other index types might work.

"comparison" is a misnomer here. Consider "binary_ops" or "logical_ops"

jreback · 2018-08-14T00:19:26Z

pandas/tests/series/test_operators.py

+        idx = Index([True, False, True, False])
+
+        expected = Series([True, False, False, False])
+        result = ser & idx


cc @jbrockmendel I think we DO allow this on index types which don't makes sense, but we DO raise on Series, e.g.

not sure what to make of [1] and [2]

In [1]: dti = pd.date_range('20130101', periods=3) In [2]: dti ^ dti Out[2]: DatetimeIndex([], dtype='datetime64[ns]', freq=None) In [3]: dti & dti Out[3]: DatetimeIndex(['2013-01-01', '2013-01-02', '2013-01-03'], dtype='datetime64[ns]', freq='D') ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all() # I think this PR fixes this one? In [4]: pd.Series(dti) & dti # this is good In [5]: pd.Series(dti) & pd.Series(dti) TypeError: cannot astype a datetimelike from [datetime64[ns]] to [bool]

I think we DO allow this on index types which don't makes sense, but we DO raise on Series,

Yah I've opened a couple of Issues about this. The existing Series ops don't handle dt64/td64 at all, and are not super-clear for some other dtypes. The reversed op(Index, Series) ops are a separate can of worms because those are set operations.

I don't think this PR fixes [4] (yet), needs to chance ABCIndex to ABCIndexClass (like #22173)

ok this is fine then (for now as you have other issues about this)

makbigc · 2018-08-14T14:57:15Z

Changing from ABCIndex to ACBIndexClass, it can handle Int64Index too. Thanks for your advice. @jbrockmendel

The tests on Int64Index are also added. @jreback

makbigc · 2018-08-20T15:18:09Z

@jreback @jbrockmendel Would you tell me if there is anything to be implemented? Or failing one of the tests does matter?

jbrockmendel · 2018-08-22T03:57:54Z

doc/source/whatsnew/v0.24.0.txt

@@ -497,6 +497,6 @@ Other
 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now takes a ``text_color_threshold`` parameter to automatically lighten the text color based on the luminance of the background color. This improves readability with dark background colors without the need to limit the background colormap range. (:issue:`21258`)
 - Require at least 0.28.2 version of ``cython`` to support read-only memoryviews (:issue:`21688`)
 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now also supports tablewise application (in addition to rowwise and columnwise) with ``axis=None`` (:issue:`15204`)
-
+- Logical operation ``&, |, ^`` between :class:`Series` and :class:`Index` will no longer raise ``ValueError`` (:issue:`22092`) 


Nitpick: pls make "operation" plural

jbrockmendel · 2018-08-22T03:58:41Z

Pending minor comments, LGTM.

makbigc · 2018-08-22T14:37:37Z

Changed. Thanks for your reply

makbigc · 2018-08-27T13:21:13Z

ping @jreback Could this be merged?

makbigc · 2018-09-04T14:20:07Z

ping @jreback @jbrockmendel Is there anything to be implemented before this PR can be merged?

jbrockmendel · 2018-09-04T17:17:19Z

Is there anything to be implemented before this PR can be merged?

@makbigc I can't speak for jreback, but rebasing will definitely be necessary

pep8speaks · 2018-09-09T10:53:18Z

Hello @makbigc! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/ops.py !
There are no PEP8 issues in the file pandas/tests/series/test_operators.py !

makbigc · 2018-09-10T14:15:12Z

@jreback do you anything else to be implemented? Please tell me.

jreback · 2018-09-15T12:27:38Z

looks good, can you rebase, ping on green.

makbigc · 2018-09-17T14:37:01Z

@jreback changed. No conflict right now.

gfyoung

Nice job!

jreback · 2018-09-18T12:53:16Z

lgtm. @jbrockmendel over to you

jreback · 2018-09-18T13:50:47Z

@jbrockmendel ok if ok, pls approve and merge

…-dev#22293) * Fix bug #GH22092 * Update v0.24.0.txt * Update v0.24.0.txt * Update ops.py * Update test_operators.py * Update v0.24.0.txt * Update test_operators.py

Fix bug #GH22092

2c1bb1a

gfyoung added Bug Dtype Conversions Unexpected or buggy dtype conversions Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff labels Aug 13, 2018

gfyoung reviewed Aug 13, 2018

View reviewed changes

jreback requested a review from jbrockmendel August 13, 2018 11:42

Update v0.24.0.txt

3607834

jbrockmendel reviewed Aug 13, 2018

View reviewed changes

jbrockmendel requested changes Aug 13, 2018

View reviewed changes

jreback requested changes Aug 14, 2018

View reviewed changes

makbigc added 3 commits August 14, 2018 22:48

Update v0.24.0.txt

9f7b586

Update ops.py

fd0f381

Update test_operators.py

f9f63fd

jbrockmendel reviewed Aug 22, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Aug 22, 2018

makbigc added 2 commits August 22, 2018 22:28

Update v0.24.0.txt

ba456e7

Update test_operators.py

84ac158

Merge branch 'master' into master

aaf8349

Merge branch 'master' into master

d671f99

gfyoung approved these changes Sep 18, 2018

View reviewed changes

jreback approved these changes Sep 18, 2018

View reviewed changes

jreback mentioned this pull request Sep 18, 2018

Fix Series v Index bool ops #22173

Merged

jbrockmendel approved these changes Sep 18, 2018

View reviewed changes

jbrockmendel merged commit 4e0b636 into pandas-dev:master Sep 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: Logical operator of Series with Index (#22092) #22293

Bug: Logical operator of Series with Index (#22092) #22293

makbigc commented Aug 12, 2018 •

edited by jreback

Loading

gfyoung Aug 13, 2018

codecov bot commented Aug 13, 2018 •

edited

Loading

jbrockmendel Aug 13, 2018

jbrockmendel Aug 13, 2018

jbrockmendel left a comment

jreback Aug 14, 2018

jbrockmendel Aug 22, 2018

jreback Aug 14, 2018

jbrockmendel Aug 14, 2018

jreback Aug 14, 2018 •

edited

Loading

makbigc commented Aug 14, 2018

makbigc commented Aug 20, 2018

jbrockmendel Aug 22, 2018

jbrockmendel commented Aug 22, 2018

makbigc commented Aug 22, 2018

makbigc commented Aug 27, 2018

makbigc commented Sep 4, 2018

jbrockmendel commented Sep 4, 2018

pep8speaks commented Sep 9, 2018

makbigc commented Sep 10, 2018

jreback commented Sep 15, 2018

makbigc commented Sep 17, 2018

gfyoung left a comment

jreback commented Sep 18, 2018

jreback commented Sep 18, 2018

Bug: Logical operator of Series with Index (#22092) #22293

Bug: Logical operator of Series with Index (#22092) #22293

Conversation

makbigc commented Aug 12, 2018 • edited by jreback Loading

Choose a reason for hiding this comment

codecov bot commented Aug 13, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel 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

jreback Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

makbigc commented Aug 14, 2018

makbigc commented Aug 20, 2018

Choose a reason for hiding this comment

jbrockmendel commented Aug 22, 2018

makbigc commented Aug 22, 2018

makbigc commented Aug 27, 2018

makbigc commented Sep 4, 2018

jbrockmendel commented Sep 4, 2018

pep8speaks commented Sep 9, 2018

makbigc commented Sep 10, 2018

jreback commented Sep 15, 2018

makbigc commented Sep 17, 2018

gfyoung left a comment

Choose a reason for hiding this comment

jreback commented Sep 18, 2018

jreback commented Sep 18, 2018

makbigc commented Aug 12, 2018 •

edited by jreback

Loading

codecov bot commented Aug 13, 2018 •

edited

Loading

jreback Aug 14, 2018 •

edited

Loading