-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
BUG: Add logical operators to pd.col Expression class #63330
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
Conversation
Add __and__, __rand__, __or__, __ror__, __xor__, __rxor__, __invert__ methods to Expression class to support combining conditions with &, |, ^, ~ operators in pd.col expressions. Closes pandas-dev#63322
rhshadrach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Can you add tests.
Add comprehensive tests for the new logical operators (__and__, __or__, __xor__, __invert__) in the Expression class: - test_col_logical_ops: Parametrized tests for &, |, ^, ~ operators with both column-to-column and column-to-scalar operations - test_col_logical_ops_combined: Integration test demonstrating real-world usage in DataFrame.loc[] with combined conditions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
rhshadrach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
pandas/tests/test_col.py
Outdated
| # __and__ and __rand__ | ||
| # a = [True, False, True, False], b = [False, True, True, True] | ||
| # a & b = [False, False, True, False] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the comments throughout the parametrization.
pandas/tests/test_col.py
Outdated
| ], | ||
| ) | ||
| def test_col_logical_ops( | ||
| expr: Expression, expected_values: list[object], expected_str: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list[bool], yea?
pandas/tests/test_col.py
Outdated
| assert str(expr) == expected_str | ||
|
|
||
|
|
||
| def test_col_logical_ops_combined() -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, can you test loc in the test above? I believe it'd just be:
result = df.loc[expr]
expected = df[expected_values]
tm.assert_frame_equal(result, expected)- Remove inline comments from parametrized test cases - Change type annotation from list[object] to list[bool] - Remove test_col_logical_ops_combined and add .loc testing to the main parametrized test function 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
rhshadrach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed one request.
| ) -> None: | ||
| df = pd.DataFrame({"a": [True, False, True, False], "b": [False, True, True, True]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ) -> None: | |
| df = pd.DataFrame({"a": [True, False, True, False], "b": [False, True, True, True]}) | |
| ) -> None: | |
| # https://github.com/pandas-dev/pandas/issues/63322 | |
| df = pd.DataFrame({"a": [True, False, True, False], "b": [False, True, True, True]}) |
|
@rhshadrach All feedback has been addressed. Could you please re-review? Thanks! |
rhshadrach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
Thanks @majiayu000! |
Summary
Add
__and__,__rand__,__or__,__ror__,__xor__,__rxor__,__invert__methods toExpressionclass to support combining conditions with&,|,^,~operators inpd.colexpressions.Example
Closes #63322