feat: add `Series.contains` #1480

FBruzzesi · 2024-12-01T15:16:26Z

What type of PR is this? (check all applicable)

Related issues

Closes enh: implement __contains__ for Series #1469

Checklist

Code follows style guide (ruff)
Tests added
Documented the changes

If you have comments or can explain your changes, please do so below

narwhals/_pandas_like/series.py

FBruzzesi · 2024-12-01T17:06:39Z

narwhals/_pandas_like/series.py

+    def __contains__(self: Self, other: Any) -> bool:
+        if other is None:
+            return self._native_series.isna().any()  # type: ignore[no-any-return]
+        return other in self._native_series


I fell for the example contained in our why section!

There are two ways of doing this:

return other in self.to_numpy(): yet I don't think this is a good idea for cudf

return self._native_series.isin({other}).any()

May I ask a general, possibly silly, question, just for the sake of better understanding? 😇

I see the point in implementing __contains__ for pandas-like and arrow and also what you mention here. Instead, I haven't properly got the details behind #1443 (comment), as I don't see func(1, pa.chunked_array([data])) returning False.

Should we implement __contains__ at narwhals/series.py to ensure the above behaviour is explicitly triggered via __contains__ (provided that what I'm stating above is correct)?

as I don't see func(1, pa.chunked_array([data])) returning False.

Damn that's true... but now I don't know why, and I wonder what is being called. The following returns false:

1 in pa.chunked_array([[1,2,3]]) False

Should we implement contains at narwhals/series.py to ensure the above behaviour is explicitly triggered via contains?

I am addressing it in the PR

Damn that's true... but now I don't know why, and I wonder what is being called.

Me neither, had issues in trying to figure it out

I am addressing it in the PR

yeah, I see it; was just asking to confirm my understanding as I couldn't indeed see where this was coming from, without the explicit reference to __contains__ 😃

Ok, since we implement __iter__ the check works. Example:

class A: pass class B: def __iter__(self): yield from [1,2,3] 0 in B(), 1 in B() > (False, True) 1 in A() > TypeError: argument of type 'A' is not iterable

When I wrote the comment, the result was false, and now it returns true due to the changes in #1471, lines. In fact before such change, iteration was returning arrow scalars

nice, thanks for the clarification!:)

MarcoGorelli · 2024-12-02T21:56:08Z

narwhals/_arrow/series.py

+        return maybe_extract_py_scalar(  # type: ignore[no-any-return]
+            pc.is_in(pa.scalar(other, type=native_series.type), native_series),


if other is of a type not convertible to native_series.type, will this raise? I'm wondering if we should return False in such a case?

looks like polars would also raise here:

In [2]: 'foo' in pl.Series([1,2,3]) --------------------------------------------------------------------------- InvalidOperationError Traceback (most recent call last) Cell In[2], line 1 ----> 1 'foo' in pl.Series([1,2,3]) File ~/scratch/.venv/lib/python3.12/site-packages/polars/series/series.py:1233, in Series.__contains__(self, item) 1231 if item is None: 1232 return self.has_nulls() -> 1233 return self.implode().list.contains(item).item() File ~/scratch/.venv/lib/python3.12/site-packages/polars/series/utils.py:106, in call_expr.<locals>.wrapper(self, *args, **kwargs) 104 expr = getattr(expr, namespace) 105 f = getattr(expr, func.__name__) --> 106 return s.to_frame().select_seq(f(*args, **kwargs)).to_series() File ~/scratch/.venv/lib/python3.12/site-packages/polars/dataframe/frame.py:9138, in DataFrame.select_seq(self, *exprs, **named_exprs) 9115 def select_seq( 9116 self, *exprs: IntoExpr | Iterable[IntoExpr], **named_exprs: IntoExpr 9117 ) -> DataFrame: 9118 """ 9119 Select columns from this DataFrame. 9120 (...) 9136 select 9137 """ -> 9138 return self.lazy().select_seq(*exprs, **named_exprs).collect(_eager=True) File ~/scratch/.venv/lib/python3.12/site-packages/polars/lazyframe/frame.py:2029, in LazyFrame.collect(self, type_coercion, predicate_pushdown, projection_pushdown, simplify_expression, slice_pushdown, comm_subplan_elim, comm_subexpr_elim, cluster_with_columns, collapse_joins, no_optimization, streaming, engine, background, _eager, **_kwargs) 2027 # Only for testing purposes 2028 callback = _kwargs.get("post_opt_callback", callback) -> 2029 return wrap_df(ldf.collect(callback)) InvalidOperationError: is_in operation not supported for dtypes `str` and `list[i64]`

probably ok to just follow them here then

still, probably good to have a test to check it raises? we can leave the expression unification for a separate topic

Will add a test and make sure that same exception is raised 👌

Ok this is definitly wrong behaviour for arrow as the casting would convert from float to int, leading to the following:

import pyarrow as pa import narwhals as nw data = [100, 200, None] s = nw.from_native(pa.table({"a": data}), eager_only=True)["a"] 100.3 in s True

On the Exception side, I would not be sure how to catch the error for pandas as:

(pd.Series([1,2,3]) == "foo").any() np.False_

In a way, I find polars behaviour more surprising

tbh we could just dispatch to (s._compliant_series == other).any() for all backends 😄 that would still be better than the status quo

MarcoGorelli · 2024-12-03T07:55:23Z

narwhals/_pandas_like/series.py

+    def __contains__(self: Self, other: Any) -> bool:
+        if other is None:
+            return self._native_series.isna().any()  # type: ignore[no-any-return]
+        return self._native_series.isin({other}).any()  # type: ignore[no-any-return]


why this over return (self._native_series == other).any()?

No strong feelings 😂 I guess I went for series.isin(...) as I initially fell for the other in series

"fell for" 😄 love that

from a quick test, it looks a lot faster

In [13]: s = pd.Series(rng.integers(0, 10, size=1_000_000)) In [14]: %timeit s.isin({0}).any() 6.58 ms ± 687 μs per loop (mean ± std. dev. of 7 runs, 100 loops each) In [15]: %timeit (s == 0).any() 326 μs ± 45.8 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

FBruzzesi · 2024-12-03T11:10:17Z

narwhals/__init__.py

-from narwhals.expr import all_ as all  # noqa: A004
+from narwhals.expr import all_ as all


Ok something must have gone wrong with the pre-commit sync :|

🤔 i'm so confused 😄

FBruzzesi · 2024-12-03T11:13:38Z

narwhals/_arrow/series.py

+                pc.is_in(other_, native_series),
+                return_py_scalar=True,
+            )
+        except (ArrowInvalid, ArrowNotImplementedError, ArrowTypeError) as exc:


Should it be bare exception?

MarcoGorelli

thanks @FBruzzesi ! and @AlessandroMiola for review

FBruzzesi · 2024-12-03T12:10:43Z

Ok then 😁 thanks @MarcoGorelli! I though you were not so happy with it 🙈

MarcoGorelli · 2024-12-03T12:40:49Z

ah sorry for giving that impression 😳

MarcoGorelli · 2024-12-03T12:48:32Z

ah perhaps this comment i left didn't help

that would still be better than the status quo

by 'status quo' i meant the main branch, not the previous commit in this pr

FBruzzesi added 5 commits December 1, 2024 14:36

feat: add Series.__contains__

e36d4ed

use pa.scalar with explicit type

e6d9419

use pa.scalar with explicit type

9a90632

use pa.scalar with explicit type

3aa5b75

Merge branch 'main' into feat/series-contains

c653e48

github-actions bot added the enhancement New feature or request label Dec 1, 2024

FBruzzesi commented Dec 1, 2024

View reviewed changes

narwhals/_pandas_like/series.py Outdated Show resolved Hide resolved

I got fooled

3f66045

FBruzzesi commented Dec 1, 2024

View reviewed changes

MarcoGorelli reviewed Dec 2, 2024

View reviewed changes

MarcoGorelli reviewed Dec 3, 2024

View reviewed changes

FBruzzesi added 4 commits December 3, 2024 11:08

Merge branch 'main' into feat/series-contains

c9f0b36

use pandas-like equals, raise custom error, deal with arrow scalar

b2b005a

arrow None

38c5833

add ArrowInvalid

634c262

FBruzzesi commented Dec 3, 2024

View reviewed changes

MarcoGorelli approved these changes Dec 3, 2024

View reviewed changes

MarcoGorelli merged commit e02685b into main Dec 3, 2024
24 checks passed

FBruzzesi deleted the feat/series-contains branch December 3, 2024 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `Series.contains` #1480

feat: add `Series.contains` #1480

FBruzzesi commented Dec 1, 2024

FBruzzesi Dec 1, 2024

AlessandroMiola Dec 1, 2024 •

edited

Loading

FBruzzesi Dec 1, 2024

AlessandroMiola Dec 1, 2024

FBruzzesi Dec 1, 2024 •

edited

Loading

AlessandroMiola Dec 1, 2024

MarcoGorelli Dec 2, 2024

MarcoGorelli Dec 3, 2024

MarcoGorelli Dec 3, 2024

FBruzzesi Dec 3, 2024 •

edited

Loading

FBruzzesi Dec 3, 2024 •

edited

Loading

MarcoGorelli Dec 3, 2024 •

edited

Loading

MarcoGorelli Dec 3, 2024

FBruzzesi Dec 3, 2024

MarcoGorelli Dec 3, 2024 •

edited

Loading

FBruzzesi Dec 3, 2024

MarcoGorelli Dec 3, 2024

FBruzzesi Dec 3, 2024

MarcoGorelli left a comment

FBruzzesi commented Dec 3, 2024

MarcoGorelli commented Dec 3, 2024

MarcoGorelli commented Dec 3, 2024

		return maybe_extract_py_scalar( # type: ignore[no-any-return]
		pc.is_in(pa.scalar(other, type=native_series.type), native_series),

		from narwhals.expr import all_ as all # noqa: A004
		from narwhals.expr import all_ as all

feat: add Series.__contains__ #1480

feat: add Series.__contains__ #1480

Conversation

FBruzzesi commented Dec 1, 2024

What type of PR is this? (check all applicable)

Related issues

Checklist

If you have comments or can explain your changes, please do so below

Choose a reason for hiding this comment

AlessandroMiola Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FBruzzesi Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FBruzzesi Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

FBruzzesi Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

MarcoGorelli Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

FBruzzesi commented Dec 3, 2024

MarcoGorelli commented Dec 3, 2024

MarcoGorelli commented Dec 3, 2024

feat: add `Series.contains` #1480

feat: add `Series.contains` #1480

AlessandroMiola Dec 1, 2024 •

edited

Loading

FBruzzesi Dec 1, 2024 •

edited

Loading

FBruzzesi Dec 3, 2024 •

edited

Loading

FBruzzesi Dec 3, 2024 •

edited

Loading

MarcoGorelli Dec 3, 2024 •

edited

Loading

MarcoGorelli Dec 3, 2024 •

edited

Loading