From 01ae613ecb6b5e90e4ef577678ae4fca01f42067 Mon Sep 17 00:00:00 2001 From: FBruzzesi Date: Wed, 13 Aug 2025 23:30:34 +0200 Subject: [PATCH 1/5] fix: Workaround for clip with modin[pyarrow] backend --- narwhals/_pandas_like/series.py | 14 ++++++++++++-- tests/expr_and_series/clip_test.py | 4 ---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/narwhals/_pandas_like/series.py b/narwhals/_pandas_like/series.py index 873680ce5b..3237537477 100644 --- a/narwhals/_pandas_like/series.py +++ b/narwhals/_pandas_like/series.py @@ -814,6 +814,7 @@ def clip( lower_bound: Self | NumericLiteral | TemporalLiteral | None, upper_bound: Self | NumericLiteral | TemporalLiteral | None, ) -> Self: + native = self.native _, lower = ( align_and_extract_native(self, lower_bound) if lower_bound is not None @@ -824,8 +825,17 @@ def clip( if upper_bound is not None else (None, None) ) - kwargs = {"axis": 0} if self._implementation is Implementation.MODIN else {} - return self._with_native(self.native.clip(lower, upper, **kwargs)) + if self._implementation.is_modin(): + result = native + if lower is not None: + result = result.where(result >= lower, lower) + if upper is not None: + result = result.where(result <= upper, upper) + + else: + result = native.clip(lower, upper) + + return self._with_native(result) def to_arrow(self) -> pa.Array[Any]: if self._implementation is Implementation.CUDF: diff --git a/tests/expr_and_series/clip_test.py b/tests/expr_and_series/clip_test.py index 23bb7af407..5f557781e9 100644 --- a/tests/expr_and_series/clip_test.py +++ b/tests/expr_and_series/clip_test.py @@ -28,8 +28,6 @@ def test_clip_expr( def test_clip_expr_expressified( request: pytest.FixtureRequest, constructor: Constructor ) -> None: - if "modin_pyarrow" in str(constructor): - request.applymarker(pytest.mark.xfail) if "cudf" in str(constructor): # https://github.com/rapidsai/cudf/issues/17682 request.applymarker(pytest.mark.xfail) @@ -66,8 +64,6 @@ def test_clip_series( def test_clip_series_expressified( request: pytest.FixtureRequest, constructor_eager: ConstructorEager ) -> None: - if "modin_pyarrow" in str(constructor_eager): - request.applymarker(pytest.mark.xfail) if "cudf" in str(constructor_eager): # https://github.com/rapidsai/cudf/issues/17682 request.applymarker(pytest.mark.xfail) From 63cedfdf3257dfb733b9b7ac4d60b3328577fd0b Mon Sep 17 00:00:00 2001 From: FBruzzesi Date: Thu, 14 Aug 2025 00:16:34 +0200 Subject: [PATCH 2/5] maintain native implementation if both lower and upper are not series --- narwhals/_pandas_like/series.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/narwhals/_pandas_like/series.py b/narwhals/_pandas_like/series.py index 3237537477..85978c909e 100644 --- a/narwhals/_pandas_like/series.py +++ b/narwhals/_pandas_like/series.py @@ -825,15 +825,19 @@ def clip( if upper_bound is not None else (None, None) ) - if self._implementation.is_modin(): - result = native - if lower is not None: - result = result.where(result >= lower, lower) - if upper is not None: - result = result.where(result <= upper, upper) - - else: + if self._implementation.is_pandas(): result = native.clip(lower, upper) + else: + native_cls = type(native) + if isinstance(lower, native_cls) or isinstance(upper, native_cls): + result = native + if lower is not None: + result = result.where(result >= lower, lower) + if upper is not None: + result = result.where(result <= upper, upper) + else: + kwargs = {"axis": 0} if self._implementation.is_modin() else {} + result = self.native.clip(lower, upper, **kwargs) return self._with_native(result) From 2e01372c9a35ec66b84fe3f22e04e5b18363132c Mon Sep 17 00:00:00 2001 From: FBruzzesi Date: Thu, 14 Aug 2025 00:26:41 +0200 Subject: [PATCH 3/5] cudf as well --- narwhals/_pandas_like/series.py | 3 +++ tests/expr_and_series/clip_test.py | 16 ++-------------- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/narwhals/_pandas_like/series.py b/narwhals/_pandas_like/series.py index 85978c909e..7c0bdd904c 100644 --- a/narwhals/_pandas_like/series.py +++ b/narwhals/_pandas_like/series.py @@ -830,6 +830,9 @@ def clip( else: native_cls = type(native) if isinstance(lower, native_cls) or isinstance(upper, native_cls): + # Workaround for both cudf and modin when clipping with a series + # * cudf: https://github.com/rapidsai/cudf/issues/17682 + # * modin: https://github.com/modin-project/modin/issues/7415 result = native if lower is not None: result = result.where(result >= lower, lower) diff --git a/tests/expr_and_series/clip_test.py b/tests/expr_and_series/clip_test.py index 5f557781e9..382133f23c 100644 --- a/tests/expr_and_series/clip_test.py +++ b/tests/expr_and_series/clip_test.py @@ -25,13 +25,7 @@ def test_clip_expr( assert_equal_data(result, {"result": expected}) -def test_clip_expr_expressified( - request: pytest.FixtureRequest, constructor: Constructor -) -> None: - if "cudf" in str(constructor): - # https://github.com/rapidsai/cudf/issues/17682 - request.applymarker(pytest.mark.xfail) - +def test_clip_expr_expressified(constructor: Constructor) -> None: data = {"a": [1, 2, 3, -4, 5], "lb": [3, 2, 1, 1, 1], "ub": [4, 4, 2, 2, 2]} df = nw.from_native(constructor(data)) result = df.select(nw.col("a").clip("lb", nw.col("ub") + 1)) @@ -61,13 +55,7 @@ def test_clip_series( assert_equal_data(result, {"result": expected}) -def test_clip_series_expressified( - request: pytest.FixtureRequest, constructor_eager: ConstructorEager -) -> None: - if "cudf" in str(constructor_eager): - # https://github.com/rapidsai/cudf/issues/17682 - request.applymarker(pytest.mark.xfail) - +def test_clip_series_expressified(constructor_eager: ConstructorEager) -> None: data = {"a": [1, 2, 3, -4, 5], "lb": [3, 2, 1, 1, 1], "ub": [4, 4, 2, 2, 2]} df = nw.from_native(constructor_eager(data), eager_only=True) result = df["a"].clip(df["lb"], df["ub"] + 1).to_frame() From 9fb5e20d00b4b27c5f27b8e8f412fb9d46e0cad6 Mon Sep 17 00:00:00 2001 From: FBruzzesi Date: Fri, 15 Aug 2025 13:53:28 +0200 Subject: [PATCH 4/5] simplify (by @dangotbanned) --- narwhals/_pandas_like/series.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/narwhals/_pandas_like/series.py b/narwhals/_pandas_like/series.py index c24b98caa8..5c8a3cc1a1 100644 --- a/narwhals/_pandas_like/series.py +++ b/narwhals/_pandas_like/series.py @@ -814,7 +814,6 @@ def clip( lower_bound: Self | NumericLiteral | TemporalLiteral | None, upper_bound: Self | NumericLiteral | TemporalLiteral | None, ) -> Self: - native = self.native _, lower = ( align_and_extract_native(self, lower_bound) if lower_bound is not None @@ -825,24 +824,24 @@ def clip( if upper_bound is not None else (None, None) ) - if self._implementation.is_pandas(): - result = native.clip(lower, upper) - else: - native_cls = type(native) + kwargs: dict[str, Any] = {} + impl = self._implementation + result = self.native + + if not impl.is_pandas(): # Workaround for both cudf and modin when clipping with a series # * cudf: https://github.com/rapidsai/cudf/issues/17682 # * modin: https://github.com/modin-project/modin/issues/7415 - result = native - if isinstance(lower, native_cls): + if impl.is_modin(): + kwargs = {"axis": 0} + if self._is_native(lower): result = result.where(result >= lower, lower) lower = None - if isinstance(upper, native_cls): + if self._is_native(upper): result = result.where(result <= upper, upper) upper = None - kwargs = {"axis": 0} if self._implementation.is_modin() else {} - result = result.clip(lower, upper, **kwargs) - return self._with_native(result) + return self._with_native(result.clip(lower, upper, **kwargs)) def to_arrow(self) -> pa.Array[Any]: if self._implementation is Implementation.CUDF: From a6921dd4b40166a4879ab31c80a6e9b93f031a36 Mon Sep 17 00:00:00 2001 From: FBruzzesi Date: Fri, 15 Aug 2025 14:03:04 +0200 Subject: [PATCH 5/5] coverage? --- narwhals/_pandas_like/series.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/narwhals/_pandas_like/series.py b/narwhals/_pandas_like/series.py index 5c8a3cc1a1..99a9baadcb 100644 --- a/narwhals/_pandas_like/series.py +++ b/narwhals/_pandas_like/series.py @@ -824,16 +824,14 @@ def clip( if upper_bound is not None else (None, None) ) - kwargs: dict[str, Any] = {} impl = self._implementation + kwargs: dict[str, Any] = {"axis": 0} if impl.is_modin() else {} result = self.native if not impl.is_pandas(): # Workaround for both cudf and modin when clipping with a series # * cudf: https://github.com/rapidsai/cudf/issues/17682 # * modin: https://github.com/modin-project/modin/issues/7415 - if impl.is_modin(): - kwargs = {"axis": 0} if self._is_native(lower): result = result.where(result >= lower, lower) lower = None