Skip to content

feat: add DataFrame.equals and Series.equals#3717

Draft
AdrianSosic wants to merge 4 commits into
narwhals-dev:mainfrom
AdrianSosic:feat/equals
Draft

feat: add DataFrame.equals and Series.equals#3717
AdrianSosic wants to merge 4 commits into
narwhals-dev:mainfrom
AdrianSosic:feat/equals

Conversation

@AdrianSosic

Copy link
Copy Markdown

Description

Just a draft for now ... Tests and docs missing!

Adds Series.equals and DataFrame.equals.

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

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

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

@AdrianSosic

Copy link
Copy Markdown
Author

@FBruzzesi: Is this what you had in mind? If yes, I could add DataFrame.equals in the same style.

@camriddell

Copy link
Copy Markdown
Member

Hi @AdrianSosic thanks for the PR to review this feature prototype. I took a look at the code and it looks like you've got everything in the right places and the implementations look solid.

Can you add some tests so we know the cases you're considering? The tests should at least encompass:

  • Comparison within all datatypes
  • Comparison across datatypes (e.g. both within and across supertypes, so int -> string; and across supertypes float -> int)
  • Comparison with Nulls (Null to Null, Null to Value)
  • Comparison with NaN (NaN to NaN, NaN to Value)

Here is a quick test I wrote against your PR for some local investigation. It's not exhaustive for the above cases, and only considers Series objects, but should at lest get you started:

Quick/incomplete Pytest code to bootstrap testing code
import narwhals as nw

import pytest


@pytest.mark.parametrize(
    ('left', 'right', 'null_equal', 'expected'),
    [
        ( [1,    2,    3],             [1,    2,    3],             True, True ),
        ( [1,    2,    3],             [1,    2,    3],             False, True ),

        ( [1.1,  2.2,  3.3],           [1.1,  2.2,  3.3],           True, True ),
        ( [1.1,  2.2,  3.3],           [1.1,  2.2,  3.3],           False, True ),

        # should we expect either of these cases to be equal?
        ( [1.1,  2,    float('nan')],  [1.1,  2,    float('nan')],  True,  True ),
        ( [1.1,  2,    float('nan')],  [1.1,  2,    float('nan')],  False, True ),

        ([1,     2,    None],          [1,    2,    3],             True, False),
        ([1,     2,    None],          [1,    2,    3],             False, False),

        ( [1,    2,    None],          [1,    2,    None],          True, True ),
        ( [1,    2,    None],          [1,    2,    None],          False, False ),

        ([1,     2,    3],             [1,    2,    0],             True, False),
        ([1,     2,    3],             [1,    2,    0],             False, False),
    ],
)
def test_equals(constructor_eager: ConstructorEager, left, right, null_equal, expected) -> 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

I have also brought up another point in the issue about NaN comparisons, so the remaining chunk of effort will be to implement that within this feature depending on what the conclusion is.

@camriddell camriddell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdrianSosic looking good so far! I've addressed most of the questions you left in the PR about expected behaviors. For now, you can leave the within supertype (e.g. int/float) comparison as-is.

For the test cases themselves, these look awesome for the numeric comparisons. Go ahead and add in cases for non-numeric datatypes (strings, dates, Categorical, etc.) as well.

Once the full test-suite is in-place, spruce up the code as much as possible. If you get stuck working on the implementation push back up and we can help iterate. Thanks so much.


def equals(
self, other: Self, *, check_dtypes: bool, check_names: bool, null_equal: bool
) -> bool:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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())

TBD = None
_NAN_HANDLING = pytest.mark.xfail(
strict=True, reason="NaN/null disambiguation not yet decided"
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NaNs should always be equivalent to one another. The only exception to this rule will be when calling equals(…, null_equal=False) on pandas objects, since it cannot disambiguate Null from NaN.

)
_CROSS_TYPE = pytest.mark.xfail(
strict=True, reason="Inconsistent behavior across backends for incompatible types"
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backends like pyarrow error out if it does not have an available kernel to perform the operation. For these cases, start with a try/except and catch the ArrowNotImplementedError. I don't believe pyarrow exposes a way to interact with the type hierarchy to check what is comparable beforehand.

Polars returns False for these cases
>>> import polars as pl
>>> pl.Series([1,2,3]).equals(pl.Series([*'123']))
False
pyarrow 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)

"Bare ChunkedArray carries no name metadata, resulting in empty names - "
"TBD whether check_names=True should raise for pyarrow in case of empty names"
),
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The names should exist on the narwhals._arrow.series.ArrowSeries object. The code seems to already have checks for names in them, unless this note was about something else?

_DTYPE_INFERENCE = pytest.mark.xfail(
strict=True,
reason="Constructor dtype inference is backend-dependent for whole-number floats",
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 convert_dtypes(convert_integer=False, dtype_backend="pyarrow"), but that causes ints to fall out of the requested dtype:

>>> 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]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you realized there was none and went ahead with #3749 🙏🏼

@FBruzzesi FBruzzesi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AdrianSosic, this is definitely moving towards the right direction. I think some details and edge cases cross typing are still missing.

In principle you could re-use/re-factor a lot of what's used in assert_{frame,series}_equal by taking out the core functionalities. Here are the helper functions I added in there were for:

  • numeric: it just uses Series.is_close
  • nested types comparison: _check_list_like and _check_struct - these are loops over rows, hence quite slow
  • categorical: _check_categorical

You might also re-use the test cases in tests/testing/assert_{frame,series}_equal_test.py to validate the implementation

return all(
self.get_column(name).equals(
other.get_column(name),
check_dtypes=False,

Copy link
Copy Markdown
Member

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

- if self.columns != other.columns:
+ if self.schema != other.schema:

cum_sum: Method[Self]
diff: Method[Self]
drop_nulls: Method[Self]
equals: Method[bool]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 Series.equals docs:

Changed in version 0.20.31: The strict parameter was renamed check_dtypes.


You can find a lot of examples in the codebase for which we align past behavior to more recent one

if self.native.isna().any() or other.native.isna().any():
return False
other_native = (
other.native.astype(self.native.dtype)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 supertype(left_dtype, right_dtype) would be quite useful

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())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does pc.equal handle upcasting internally?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enh]: Add support for {DataFrame, Series}.equals

4 participants