Skip to content
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

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

Merged
merged 5 commits into from Apr 12, 2019

Conversation

@alexcwatt
Copy link
Contributor

commented Mar 30, 2019

  • closes #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').

@codecov

This comment has been minimized.

Copy link

commented Mar 30, 2019

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Copy link

commented Mar 30, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

left a comment

Can you add a whatsnew note for v0.25?

# 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

This comment has been minimized.

Copy link
@WillAyd

WillAyd Mar 30, 2019

Member

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

This comment has been minimized.

Copy link
@alexcwatt

alexcwatt Mar 30, 2019

Author Contributor

Good catch, removing this.

@@ -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)})

This comment has been minimized.

Copy link
@WillAyd

WillAyd Mar 30, 2019

Member

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

This comment has been minimized.

Copy link
@alexcwatt

alexcwatt Mar 30, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@WillAyd

WillAyd Mar 30, 2019

Member

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

This comment has been minimized.

Copy link
@jreback

jreback Mar 30, 2019

Contributor

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

This comment has been minimized.

Copy link
@alexcwatt

alexcwatt Mar 30, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@alexcwatt

alexcwatt Mar 30, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@alexcwatt

alexcwatt Mar 30, 2019

Author Contributor

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

alexcwatt added some commits Mar 30, 2019

@@ -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`)

This comment has been minimized.

Copy link
@jreback

jreback Mar 30, 2019

Contributor

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

This comment has been minimized.

Copy link
@alexcwatt

alexcwatt Mar 30, 2019

Author Contributor

Yep, will do.

@@ -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)})

This comment has been minimized.

Copy link
@jreback

jreback Mar 30, 2019

Contributor

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

@alexcwatt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@jreback Should be ready for another review.

@jreback jreback added this to the 0.25.0 milestone Apr 12, 2019

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

11 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20190330.28 succeeded
Details
pandas-dev.pandas (Checks_and_doc) Checks_and_doc succeeded
Details
pandas-dev.pandas (Linux py35_compat) Linux py35_compat succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow) Linux py36_locale_slow succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow_old_np) Linux py36_locale_slow_old_np succeeded
Details
pandas-dev.pandas (Linux py37_locale) Linux py37_locale succeeded
Details
pandas-dev.pandas (Linux py37_np_dev) Linux py37_np_dev succeeded
Details
pandas-dev.pandas (Windows py36_np15) Windows py36_np15 succeeded
Details
pandas-dev.pandas (Windows py37_np141) Windows py37_np141 succeeded
Details
pandas-dev.pandas (macOS py35_macos) macOS py35_macos succeeded
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

thanks @alexcwatt

@kenahoo

This comment has been minimized.

Copy link

commented Apr 12, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

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

This comment has been minimized.

Copy link

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

plan make a new issue instead

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.