-
Notifications
You must be signed in to change notification settings - Fork 210
feat: add DataFrame.equals and Series.equals #3717
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 4 commits
2dfd834
fdec695
5f5af7e
faed3fc
6a6fa3a
deb5259
a89ce90
8b54202
b9c2326
16c7980
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 |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| - columns | ||
| - drop | ||
| - drop_nulls | ||
| - equals | ||
| - estimated_size | ||
| - explode | ||
| - filter | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| - diff | ||
| - drop_nulls | ||
| - dtype | ||
| - equals | ||
| - ewm_mean | ||
| - exp | ||
| - fill_nan | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -553,6 +553,26 @@ def cast(self, dtype: IntoDType) -> Self: | |
| def null_count(self, *, _return_py_scalar: bool = True) -> int: | ||
| return maybe_extract_py_scalar(self.native.null_count, _return_py_scalar) | ||
|
|
||
| def equals( | ||
| self, other: Self, *, check_dtypes: bool, check_names: bool, null_equal: bool | ||
| ) -> bool: | ||
|
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. This should also compare NaNs, which will always be equivalent to one another. You could do something like lhs = ...
rhs = ...
try:
both_nan = pc.fill_null(pc.and_(pc.is_nan(lhs), pc.is_nan(rhs)), False)
except ArrowNotImplementedError:
both_nan = pa.scalar(False) # may need to broadcast this back out to a full array?
values_equal = pc.equal(lhs, rhs)
values_or_nan_equal = pc.fill_null(pc.or_(values_equal, both_nan), False) # no nulls in this array
if null_equal:
# make the extra comparison with null values if null_equal is True
....
return ...
return bool(pc.all(values_or_nan_equal).as_py())
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. @camriddell: Thanks for the suggestion ππΌ Note that I slightly rewrote so that the question of broadcasting doesn't even arise in the first place |
||
| if check_names and self._name != other._name: | ||
| return False | ||
| if check_dtypes and self.dtype != other.dtype: | ||
| return False | ||
| if len(self) != len(other): | ||
| return False | ||
| lhs = self.native | ||
| rhs = other.native | ||
| if null_equal: | ||
| both_null = pc.and_(pc.is_null(lhs), pc.is_null(rhs)) | ||
| values_equal = pc.equal(lhs, rhs) | ||
| result = pc.if_else(both_null, True, values_equal) | ||
| return bool(pc.all(result, skip_nulls=False).as_py()) | ||
| if lhs.null_count or rhs.null_count: | ||
| return False | ||
| return bool(pc.all(pc.equal(lhs, rhs)).as_py()) | ||
|
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. Does
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. yep >>> import pyarrow as pa
>>> import pyarrow.compute as pc
>>> pc.equal(pa.array([1,2,3], type=pa.int64()), pa.array([1.0, 2.0, 3.0], type=pa.float64()))
<pyarrow.lib.BooleanArray object at 0x7f46f53ceda0>
[
true,
true,
true
]
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. so no TODO here, right? |
||
|
|
||
| def head(self, n: int) -> Self: | ||
| if n >= 0: | ||
| return self._with_native(self.native.slice(0, n)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1219,6 +1219,25 @@ def struct(self) -> PandasLikeSeriesStructNamespace: | |
| raise TypeError(msg) | ||
| return PandasLikeSeriesStructNamespace(self) | ||
|
|
||
| def equals( | ||
| self, other: Self, *, check_dtypes: bool, check_names: bool, null_equal: bool | ||
| ) -> bool: | ||
| if check_names and self._name != other._name: | ||
| return False | ||
| if check_dtypes and self.dtype != other.dtype: | ||
| return False | ||
| if not null_equal: | ||
| if len(self) != len(other): | ||
| return False # not needed but can short-circuit the isna() checks below | ||
| if self.native.isna().any() or other.native.isna().any(): | ||
| return False | ||
| other_native = ( | ||
| other.native.astype(self.native.dtype) | ||
|
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. Not sure how I feel about this, can't downcasting potentially raise an error? This is where a
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. Agreed we should avoid manually casting here. >>> import pandas as pd
>>> self = pd.Series([1, 2, 3])
>>> other = pd.Series([*'123'])
>>> self.equals(other)
False
>>> self.equals(other.astype(self.dtype))
True
dtype: int64
>>> other.astype(self.dtype)
0 1
1 2
2 3Instead of directly using >>> pd.Series([1,2,3], dtype="int64") == pd.Series([1.0, 2.0, 3.0], dtype="float64")
0 True
1 True
2 True
dtype: bool
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 point! But I guess the
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. Sorry for the "not enough" context - that's correct get_supertype is still not here. Hopefully it will eventually get merged #3396
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. Ok, so I guess I'll be waiting then until this happens
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.
Please don't! There is not enough consensus on it for now - I brought it up as a nice use case for it, not for blocking you |
||
| if not check_dtypes and self.native.dtype != other.native.dtype | ||
| else other.native | ||
| ) | ||
| return self.native.equals(other_native) | ||
|
|
||
|
|
||
| class _PandasHist(EagerSeriesHist["pd.Series[Any]", "list[float]"]): | ||
| _series: PandasLikeSeries | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,7 @@ | |
| "cum_sum", | ||
| "diff", | ||
| "drop_nulls", | ||
| "equals", | ||
| "exp", | ||
| "fill_null", | ||
| "fill_nan", | ||
|
|
@@ -720,6 +721,7 @@ def struct(self) -> PolarsSeriesStructNamespace: | |
| cum_sum: Method[Self] | ||
| diff: Method[Self] | ||
| drop_nulls: Method[Self] | ||
| equals: Method[bool] | ||
|
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. You might need to actually implement the method for backward compat. From the
You can find a lot of examples in the codebase for which we align past behavior to more recent one
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. |
||
| exp: Method[Self] | ||
| fill_null: Method[Self] | ||
| fill_nan: Method[Self] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| import pytest | ||
|
|
||
| import narwhals as nw | ||
|
|
||
| if TYPE_CHECKING: | ||
| from tests.utils import ConstructorEager | ||
|
|
||
| TBD = None | ||
| _NAN_HANDLING = pytest.mark.xfail( | ||
| strict=True, reason="NaN/null disambiguation not yet decided" | ||
| ) | ||
|
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.
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. I understand the problem, but not sure if I'm 100% what is the desired behavior for this case now. For the arrow backend, the problem could in principle be resolved, but not for the numpy backend. So what do we do?
And then, regarding the failing case(s) that remain:
|
||
| _CROSS_TYPE = pytest.mark.xfail( | ||
| strict=True, reason="Inconsistent behavior across backends for incompatible types" | ||
| ) | ||
|
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. Backends like Polars returns False for these cases>>> import polars as pl
>>> pl.Series([1,2,3]).equals(pl.Series([*'123']))
Falsepyarrow raises errors when working with un-related dtypes>>> import pyarrow.compute as pc
>>> import pyarrow as pa
>>> import pyarrow.compute as pc
>>> pc.equal(pa.array([1,2,3]), pa.array([*'123']))
Traceback (most recent call last):
File "<python-input-5>", line 1, in <module>
pc.equal(pa.array([1,2,3]), pa.array([*'123']))
~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/cameron/.pyenv/versions/3.14.2/lib/python3.14/site-packages/pyarrow/compute.py", line 252, in wrapper
return func.call(args, None, memory_pool)
~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
File "pyarrow/_compute.pyx", line 399, in pyarrow._compute.Function.call
File "pyarrow/error.pxi", line 155, in pyarrow.lib.pyarrow_internal_check_status
File "pyarrow/error.pxi", line 92, in pyarrow.lib.check_status
pyarrow.lib.ArrowNotImplementedError: Function 'equal' has no kernel matching input types (int64, string)
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. |
||
| _DTYPE_INFERENCE = pytest.mark.xfail( | ||
| strict=True, | ||
| reason="Constructor dtype inference is backend-dependent for whole-number floats", | ||
| ) | ||
|
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. @FBruzzesi are there any test helpers that can help with this problem? We want the constructors to NOT eagerly downcast values. For example, these should each end up as floats dtype. >>> pd.Series([1.0])
0 1.0
dtype: float64
>>> pd.Series([1.0]).convert_dtypes(dtype_backend="numpy_nullable")
0 1
dtype: Int64
>>> pd.Series([1.0]).convert_dtypes(dtype_backend="pyarrow")
0 1
dtype: int64[pyarrow]There is >>> pd.Series([1]).convert_dtypes(convert_integer=False, dtype_backend="pyarrow").dtype
dtype('int64')
>>> pd.Series([1.0]).convert_dtypes(convert_integer=False, dtype_backend="pyarrow").dtype
double[pyarrow]
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. I assume you realized there was none and went ahead with #3749 ππΌ
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. |
||
| _CHECK_NAMES = pytest.mark.xfail( | ||
| strict=True, | ||
| reason=( | ||
| "Bare ChunkedArray carries no name metadata, resulting in empty names - " | ||
| "TBD whether check_names=True should raise for pyarrow in case of empty names" | ||
| ), | ||
| ) | ||
|
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. The names should exist on the
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. Yes, the import pyarrow as pa
import narwhals as nw
t = pa.table({"left": [1, 2], "right": [1, 2]})
left = nw.from_native(t["left"], series_only=True)
right = nw.from_native(t["right"], series_only=True)
print(left.name) # ""
print(right.name) # ""
print(left.equals(right, check_names=True)) # True β names are both "", so check passes silently
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, I see the where the issue arises now. Can you rewrite the test in this fashion so the metadata is preserved appropriately? |
||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("left", "right", "expected"), | ||
| [ | ||
| # --- Same dtype: int --- | ||
| ([1, 2], [1, 2], True), | ||
| # --- Same dtype: float --- | ||
| ([1.1, 2.2], [1.1, 2.2], True), | ||
| # --- Value differs --- | ||
| ([1, 2], [1, 0], False), | ||
| # --- Cross-supertype: float vs int --- | ||
| ([1.0, 2.0], [1, 2], True), | ||
| ([1.1, 2.2], [1, 2], False), | ||
| # --- Cross-supertype: int vs string --- | ||
| # pandas: False, polars: False, pyarrow: raises | ||
| pytest.param([1, 2], ["1", "2"], TBD, marks=_CROSS_TYPE), | ||
| ], | ||
| ) | ||
| def test_series_equals( | ||
| constructor_eager: ConstructorEager, | ||
| left: list, | ||
| right: list, | ||
| expected: bool | None, # noqa: FBT001 | ||
| ) -> None: | ||
| left_native = constructor_eager({"left": left})["left"] | ||
| right_native = constructor_eager({"right": right})["right"] | ||
| left_nw = nw.from_native(left_native, series_only=True) | ||
| right_nw = nw.from_native(right_native, series_only=True) | ||
|
|
||
| result = left_nw.equals(right_nw) | ||
| assert result == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("left", "right", "null_equal", "expected"), | ||
| [ | ||
| # --- Null vs value --- | ||
| ([1, None], [1, 2], True, False), | ||
| ([1, None], [1, 2], False, False), | ||
| # --- Null vs Null --- | ||
| ([1, None], [1, None], True, True), | ||
| ([1, None], [1, None], False, False), | ||
| # --- NaN vs NaN --- | ||
| # pandas: True, polars: True, pyarrow: False | ||
| pytest.param( | ||
| [1.0, float("nan")], [1.0, float("nan")], True, TBD, marks=_NAN_HANDLING | ||
| ), | ||
| # pandas: True, polars: True, pyarrow: False | ||
| pytest.param( | ||
| [1.0, float("nan")], [1.0, float("nan")], False, TBD, marks=_NAN_HANDLING | ||
| ), | ||
| # --- NaN vs value --- | ||
| # pandas: False, polars: False, pyarrow: False | ||
| ([1.0, float("nan")], [1.0, 2.0], True, False), | ||
| ], | ||
| ) | ||
| def test_series_equals_null_equal( | ||
| constructor_eager: ConstructorEager, | ||
| left: list, | ||
| right: list, | ||
| null_equal: bool, # noqa: FBT001 | ||
| expected: bool | None, # noqa: FBT001 | ||
| ) -> None: | ||
| left_native = constructor_eager({"left": left})["left"] | ||
| right_native = constructor_eager({"right": right})["right"] | ||
| left_nw = nw.from_native(left_native, series_only=True) | ||
| right_nw = nw.from_native(right_native, series_only=True) | ||
|
|
||
| result = left_nw.equals(right_nw, null_equal=null_equal) | ||
| assert result == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("left", "right", "check_dtypes", "expected"), | ||
| [ | ||
| # --- Same values, different dtype --- | ||
| ([1.1, 2.2], [1, 2], False, False), | ||
| # --- Whole-number floats, check_dtypes=False --- | ||
| ([1.0, 2.0], [1, 2], False, True), | ||
| # --- Whole-number floats, check_dtypes=True --- | ||
| # pandas: False, polars: False, pyarrow: False, pandas[pyarrow]: True (infers Int64) | ||
| pytest.param([1.0, 2.0], [1, 2], True, TBD, marks=_DTYPE_INFERENCE), | ||
| ], | ||
| ) | ||
| def test_series_equals_check_dtypes( | ||
| constructor_eager: ConstructorEager, | ||
| left: list, | ||
| right: list, | ||
| check_dtypes: bool, # noqa: FBT001 | ||
| expected: bool | None, # noqa: FBT001 | ||
| ) -> None: | ||
| left_native = constructor_eager({"a": left})["a"] | ||
| right_native = constructor_eager({"a": right})["a"] | ||
| left_nw = nw.from_native(left_native, series_only=True) | ||
| right_nw = nw.from_native(right_native, series_only=True) | ||
|
|
||
| result = left_nw.equals(right_nw, check_dtypes=check_dtypes) | ||
| assert result == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("left_name", "right_name", "check_names", "expected"), | ||
| [ | ||
| # --- Different names --- | ||
| ("left", "right", False, True), | ||
| # pandas: False, polars: False, pyarrow: True (ChunkedArray has no name metadata) | ||
| pytest.param("left", "right", True, TBD, marks=_CHECK_NAMES), | ||
| ], | ||
| ) | ||
| def test_series_equals_check_names( | ||
| constructor_eager: ConstructorEager, | ||
| left_name: str, | ||
| right_name: str, | ||
| check_names: bool, # noqa: FBT001 | ||
| expected: bool | None, # noqa: FBT001 | ||
| ) -> None: | ||
| data = [1, 2] | ||
| left_native = constructor_eager({left_name: data})[left_name] | ||
| right_native = constructor_eager({right_name: data})[right_name] | ||
| left_nw = nw.from_native(left_native, series_only=True) | ||
| right_nw = nw.from_native(right_native, series_only=True) | ||
|
|
||
| result = left_nw.equals(right_nw, check_names=check_names) | ||
| assert result == expected | ||
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.
Shouldn't we check also the dtype match? I think it's ok to turn off the check as long as above
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.
8b54202