BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp #25928

alexcwatt · 2019-03-30T04:31:22Z

closes DataFrame.eval errors with AttributeError: 'UnaryOp' #16363
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Checking hasattr(side, 'value') resolves the bug and is consistent with the way that other methods of the BaseExprVisitor class work (e.g., both visit_Call_35 and visit_Call_legacy check for 'value').

…on UnaryOp

codecov · 2019-03-30T05:09:53Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #25928      +/-   ##
==========================================
- Coverage    91.8%   91.79%   -0.01%     
==========================================
  Files         174      174              
  Lines       52536    52536              
==========================================
- Hits        48231    48226       -5     
- Misses       4305     4310       +5

Flag	Coverage Δ
#multiple	`90.35% <100%> (ø)`	⬆️
#single	`41.88% <100%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/computation/expr.py	`88.52% <100%> (ø)`	⬆️
pandas/io/gbq.py	`75% <0%> (-12.5%)`	⬇️
pandas/core/frame.py	`96.79% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.63% <0%> (-0.11%)`	⬇️

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 1d4c89f...ef777e7. Read the comment docs.

codecov · 2019-03-30T05:09:53Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #25928      +/-   ##
==========================================
- Coverage    91.8%    91.8%   -0.01%     
==========================================
  Files         174      175       +1     
  Lines       52536    52578      +42     
==========================================
+ Hits        48231    48269      +38     
- Misses       4305     4309       +4

Flag	Coverage Δ
#multiple	`90.36% <100%> (ø)`	⬆️
#single	`41.9% <100%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/computation/expr.py	`88.52% <100%> (ø)`	⬆️
pandas/io/gbq.py	`75% <0%> (-12.5%)`	⬇️
pandas/core/frame.py	`96.79% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.63% <0%> (-0.11%)`	⬇️
pandas/core/dtypes/base.py	`100% <0%> (ø)`	⬆️
pandas/core/groupby/base.py	`91.83% <0%> (ø)`	⬆️
pandas/core/arrays/array_.py	`100% <0%> (ø)`	⬆️
pandas/io/gcs.py	`80% <0%> (ø)`	⬆️
pandas/core/groupby/groupby.py	`97.21% <0%> (ø)`	⬆️
pandas/core/internals/managers.py	`93.93% <0%> (ø)`	⬆️
... and 15 more

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 1d4c89f...1b88337. Read the comment docs.

WillAyd

Can you add a whatsnew note for v0.25?

WillAyd · 2019-03-30T05:21:40Z

pandas/tests/computation/test_eval.py

+        # GH 16363
+        df = pd.DataFrame({'x': np.array([0], dtype=np.float32)})
+        res = df.eval('x < -0.1')
+        assert np.array_equal(res, np.array([False])), res


What's the reason for , res here? I think also causing CI failure

Good catch, removing this.

WillAyd · 2019-03-30T05:23:50Z

pandas/tests/computation/test_eval.py

@@ -608,6 +608,12 @@ def test_unary_in_array(self):
                      -False, False, ~False, +False,
                      -37, 37, ~37, +37], dtype=np.object_))

+    def test_float_comparison_bin_op(self):
+        # GH 16363
+        df = pd.DataFrame({'x': np.array([0], dtype=np.float32)})


Is this specific to float32? Read briefly through issue and seemed like it affected other things

@WillAyd The bug arises when one side returns a float32 and the other side is a scalar that doesn't have a 'value' attribute.

Does this still work if the operands are switched? i.e. -0.1 > x?

an you parameterize on the dtype (at least float32/float64)

Does this still work if the operands are switched? i.e. -0.1 > x?

I will add a test with a negative value on the left.

an you parameterize on the dtype (at least float32/float64)

Would you like a test for float64? This particular bug was caused by code that handles float32's, so float64's were fine, but I can add a test to ensure that this does not become an issue.

@jreback Oh, I see now pytest's parametrize in some of the other tests, I will update this

jreback · 2019-03-30T17:03:21Z

doc/source/whatsnew/v0.25.0.rst

@@ -228,6 +228,7 @@ Performance Improvements
 Bug Fixes
 ~~~~~~~~~

+- Bug in :meth:`~pandas.eval` when comparing floats with scalar operators that don't have a 'value' attribute (e.g., unary operators) (:issue:`25928`)


can you move down to numeric section.

make this more like:

Bug in :meth:`~pandas.eval` when comparing floats with scalar operators, for example: ``x < 0.1`` (:issue:...)

Yep, will do.

jreback · 2019-03-30T17:04:33Z

pandas/tests/computation/test_eval.py

@@ -608,6 +608,12 @@ def test_unary_in_array(self):
                      -False, False, ~False, +False,
                      -37, 37, ~37, +37], dtype=np.object_))

+    def test_float_comparison_bin_op(self):
+        # GH 16363
+        df = pd.DataFrame({'x': np.array([0], dtype=np.float32)})


an you parameterize on the dtype (at least float32/float64)

alexcwatt · 2019-04-11T01:20:21Z

@jreback Should be ready for another review.

jreback · 2019-04-12T11:52:21Z

thanks @alexcwatt

kenahoo · 2019-04-12T18:27:55Z

@jreback & @alexcwatt - this was also an issue for string data, as I showed in this previous comment on #16363 :

#16363 (comment)

The test case is:

def test_unary():
    df = pd.DataFrame({'x': ["one", "two"]})
    df.eval('x.shift(-1)')

Is that also fixed now? If not, we should keep #16363 open as not fixed.

jreback · 2019-04-12T18:53:38Z

you can test on master and if not pls open. new issue

if it works then can add an additional test (pls do a PR)

kenahoo · 2019-04-12T19:06:49Z

I don't think I'll have time for that very soon, I'd recommend just re-opening #16363 until the full set of problems identified in the ticket can be verified as fixed.

jreback · 2019-04-12T20:18:52Z

plan make a new issue instead

…on UnaryOp (pandas-dev#25928)

BUG: Fix pandas-dev#16363 - Prevent visit_BinOp from accessing value …

ef777e7

…on UnaryOp

WillAyd requested changes Mar 30, 2019

View reviewed changes

alexcwatt added 2 commits March 30, 2019 08:19

Fix test and add note to whatsnew

5242bef

Attempt to fix test style

f0e0b3d

jreback added Bug Numeric Operations Arithmetic, Comparison, and Logical operations labels Mar 30, 2019

jreback requested changes Mar 30, 2019

View reviewed changes

alexcwatt added 2 commits March 30, 2019 18:38

Update whatsnew and add another test case

5d6ea06

Use parametrize to test both float32 and float64

1b88337

WillAyd approved these changes Mar 30, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Apr 12, 2019

jreback approved these changes Apr 12, 2019

View reviewed changes

jreback merged commit d062858 into pandas-dev:master Apr 12, 2019

yhaque1213 pushed a commit to yhaque1213/pandas that referenced this pull request Apr 22, 2019

BUG: Fix pandas-dev#16363 - Prevent visit_BinOp from accessing value …

fad68eb

…on UnaryOp (pandas-dev#25928)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp #25928

BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp #25928

alexcwatt commented Mar 30, 2019

codecov bot commented Mar 30, 2019

codecov bot commented Mar 30, 2019 •

edited

Loading

WillAyd left a comment

WillAyd Mar 30, 2019

alexcwatt Mar 30, 2019

WillAyd Mar 30, 2019

alexcwatt Mar 30, 2019

WillAyd Mar 30, 2019

jreback Mar 30, 2019

alexcwatt Mar 30, 2019

alexcwatt Mar 30, 2019

alexcwatt Mar 30, 2019

jreback Mar 30, 2019

alexcwatt Mar 30, 2019

jreback Mar 30, 2019

alexcwatt commented Apr 11, 2019

jreback commented Apr 12, 2019

kenahoo commented Apr 12, 2019

jreback commented Apr 12, 2019

kenahoo commented Apr 12, 2019

jreback commented Apr 12, 2019

BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp #25928

BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp #25928

Conversation

alexcwatt commented Mar 30, 2019

codecov bot commented Mar 30, 2019

Codecov Report

codecov bot commented Mar 30, 2019 • edited Loading

Codecov Report

WillAyd 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

alexcwatt commented Apr 11, 2019

jreback commented Apr 12, 2019

kenahoo commented Apr 12, 2019

jreback commented Apr 12, 2019

kenahoo commented Apr 12, 2019

jreback commented Apr 12, 2019

codecov bot commented Mar 30, 2019 •

edited

Loading