-
Notifications
You must be signed in to change notification settings - Fork 210
feat: add Selector.__xor__ and Expr.__xor__ for symmetric difference
#3621
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
81c6263
35e9fcf
c8e6cf9
94d6214
1cb834e
dd5f155
b742f41
e56006d
2e943a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,22 @@ def __and__(self, other: Any) -> Expr: # type: ignore[override] | |
| ExprNode(ExprKind.ELEMENTWISE, "__and__", exprs=(other,), str_as_lit=True) | ||
| ) | ||
|
|
||
| def __xor__(self, other: Any) -> Expr: | ||
| if isinstance(other, Selector): | ||
| return self._append_node( | ||
| ExprNode( | ||
| ExprKind.ELEMENTWISE, | ||
| "__xor__", | ||
| exprs=(other,), | ||
| str_as_lit=True, | ||
| allow_multi_output=True, | ||
| ) | ||
| ) | ||
| msg = ( | ||
| f"unsupported operand type(s) for op: ('Selector' ^ '{type(other).__name__}')" | ||
| ) | ||
| raise TypeError(msg) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in why are we raising an error here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah thanks - i kinda feel like, if we're adding i'd suggest
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MarcoGorelli circling back on your review thread (3269698893): I went with option A and added |
||
|
|
||
| def __rsub__(self, other: Any) -> NoReturn: | ||
| raise NotImplementedError | ||
|
|
||
|
|
@@ -65,6 +81,9 @@ def __rand__(self, other: Any) -> NoReturn: | |
| def __ror__(self, other: Any) -> NoReturn: | ||
| raise NotImplementedError | ||
|
|
||
| def __rxor__(self, other: Any) -> NoReturn: | ||
| raise NotImplementedError | ||
|
|
||
|
|
||
| def by_dtype(*dtypes: DType | type[DType] | Iterable[DType | type[DType]]) -> Selector: | ||
| """Select columns based on their dtype. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,11 @@ def test_comparand_operators_expr( | |
|
|
||
| @pytest.mark.parametrize( | ||
| ("operator", "expected"), | ||
| [("__and__", [True, False, False, False]), ("__or__", [True, True, True, False])], | ||
| [ | ||
| ("__and__", [True, False, False, False]), | ||
| ("__or__", [True, True, True, False]), | ||
| ("__xor__", [False, True, True, False]), | ||
| ], | ||
| ) | ||
| def test_logic_operators_expr( | ||
| constructor: Constructor, operator: str, expected: list[bool] | ||
|
|
@@ -60,6 +64,18 @@ def test_logic_operators_expr( | |
| assert_equal_data(result, {"a": expected}) | ||
|
|
||
|
|
||
| def test_xor_operator_expr(constructor: Constructor) -> None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also have a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in commit c8e6cf9. The test covers True/True, True/None, False/None, None/None cases and checks that null propagates for pandas-style constructors differently (pandas treats nulls as False in boolean ops). |
||
| data = {"a": [True, False, True, False], "b": [True, True, False, False]} | ||
| df = nw.from_native(constructor(data)) | ||
| result = df.select(a=nw.col("a") ^ nw.col("b")).lazy().collect() | ||
| column = result.get_column("a").to_list() | ||
| assert len(column) == 4 | ||
| assert column == [False, True, True, False] | ||
| # Default __and__ still produces the conjunction. | ||
| and_result = df.select(a=nw.col("a") & nw.col("b")).lazy().collect() | ||
| assert and_result.get_column("a").to_list() == [True, False, False, False] | ||
|
|
||
|
|
||
| def test_logic_operators_expr_kleene( | ||
| constructor: Constructor, request: pytest.FixtureRequest | ||
| ) -> None: | ||
|
|
@@ -92,6 +108,8 @@ def test_logic_operators_expr_kleene( | |
| ("__rand__", [False, False, False, False]), | ||
| ("__or__", [True, True, False, False]), | ||
| ("__ror__", [True, True, False, False]), | ||
| ("__xor__", [True, True, False, False]), | ||
| ("__rxor__", [True, True, False, False]), | ||
| ], | ||
| ) | ||
| def test_logic_operators_expr_scalar( | ||
|
|
@@ -103,7 +121,7 @@ def test_logic_operators_expr_scalar( | |
| if ( | ||
| "dask" in str(constructor) | ||
| and DASK_VERSION < (2024, 10) | ||
| and operator in {"__rand__", "__ror__"} | ||
| and operator in {"__rand__", "__ror__", "__rxor__"} | ||
| ): | ||
| request.applymarker(pytest.mark.xfail) | ||
| data = {"a": [True, True, False, False]} | ||
|
|
@@ -161,6 +179,8 @@ def test_comparand_operators_series( | |
| ("__rand__", [True, False, False, False]), | ||
| ("__or__", [True, True, True, False]), | ||
| ("__ror__", [True, True, True, False]), | ||
| ("__xor__", [False, True, True, False]), | ||
| ("__rxor__", [False, True, True, False]), | ||
| ], | ||
| ) | ||
| def test_logic_operators_series( | ||
|
|
||
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.
is this still needed if you've implemented it for
Expr?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.
Not anymore, no. In 2e943a2 it falls back to
self._to_expr() ^ otherlike__and__/__or__, and the narwhals-levelSelector.__xor__does the same instead of raising; added atest_xor_exprcase. Polars needed a small tweak since it coerces the right operand to a selector and takes the set difference.