BUG: Fix #16363 - Prevent visit_BinOp from accessing value on UnaryOp by alexcwatt · Pull Request #25928 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation20 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- 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').
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for , res here? I think also causing CI failure
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)}) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this specific to float32? Read briefly through issue and seemed like it affected other things
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd The bug arises when one side returns a float32 and the other side is a scalar that doesn't have a 'value' attribute.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still work if the operands are switched? i.e. -0.1 > x?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an you parameterize on the dtype (at least float32/float64)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback Oh, I see now pytest's parametrize in some of the other tests, I will update this
| @@ -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`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:...)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)}) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an you parameterize on the dtype (at least float32/float64)
@jreback Should be ready for another review.
@jreback & @alexcwatt - this was also an issue for string data, as I showed in this previous comment on #16363 :
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.
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)
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.
plan make a new issue instead
yhaque1213 pushed a commit to yhaque1213/pandas that referenced this pull request
