Skip to content

BUG/API: other object type check in Series/DataFrame.equals #34402

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jul 15, 2020
Merged

BUG/API: other object type check in Series/DataFrame.equals #34402

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
63d7825
BUG/API: other object type check in Series/DataFrame.equals
jorisvandenbossche May 27, 2020
75f5749
Merge remote-tracking branch 'upstream/master' into equals-constructor
jorisvandenbossche May 29, 2020
f148449
allow subclass + add test
jorisvandenbossche May 29, 2020
df88cb8
Merge remote-tracking branch 'upstream/master' into equals-constructor
jorisvandenbossche Jun 19, 2020
348f706
use SubclassedSeries
jorisvandenbossche Jun 19, 2020
1759221
add whatsnew
jorisvandenbossche Jun 19, 2020
f530286
Merge remote-tracking branch 'upstream/master' into equals-constructor
jorisvandenbossche Jul 3, 2020
66293a5
add dataframe test
jorisvandenbossche Jul 3, 2020
bcca6b7
Merge remote-tracking branch 'upstream/master' into equals-constructor
TomAugspurger Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/source/whatsnew/v1.1.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1151,6 +1151,8 @@ Other
- Bug in :meth:`DataFrame.__dir__` caused a segfault when using unicode surrogates in a column name (:issue:`25509`)
- Bug in :meth:`DataFrame.plot.scatter` caused an error when plotting variable marker sizes (:issue:`32904`)
- :class:`IntegerArray` now implements the ``sum`` operation (:issue:`33172`)
- Bug in :meth:`DataFrame.equals` and :meth:`Series.equals` in allowing subclasses
to be equal (:issue:`34402`).
- Bug in :class:`Tick` comparisons raising ``TypeError`` when comparing against timedelta-like objects (:issue:`34088`)
- Bug in :class:`Tick` multiplication raising ``TypeError`` when multiplying by a float (:issue:`34486`)
- Passing a `set` as `names` argument to :func:`pandas.read_csv`, :func:`pandas.read_table`, or :func:`pandas.read_fwf` will raise ``ValueError: Names should be an ordered collection.`` (:issue:`34946`)
Expand Down
2 changes: 1 addition & 1 deletion pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,7 @@ def equals(self, other):
>>> df.equals(different_data_type)
False
"""
if not isinstance(other, self._constructor):
if not (isinstance(other, type(self)) or isinstance(self, type(other))):
return False
return self._mgr.equals(other._mgr)

Expand Down
14 changes: 14 additions & 0 deletions pandas/tests/generic/test_generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,6 +872,20 @@ def test_equals(self):
assert a.equals(e)
assert e.equals(f)

def test_equals_subclass(self):
# https://github.com/pandas-dev/pandas/pull/34402
# allow subclass in both directions

Copy link
Contributor

Choose a reason for hiding this comment

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

can you put this in tests/series/test_sublcass and similar for frame/test_sublcass

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not a test about the subclass working correctly (anything related to the subclassing machinery, eg correct use of _constructor to preserve the subclass in operations etc), but only about our own equals method accepting subclasses, so I think the test rather fits here with the other equals tests.

But I can certainly use the tm.SubclassedSeries helper to avoid creating a dummy class here.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this belongs with the rest of the tests & needs for DataFrame subclass as well. having 2 places is just very friendly to future folks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having 2 places where equals is tested is equally unfriendly I think ..
Both ways have some aspect of the tests splitted in two places, and as mentioned above, this test is actually about our equals implementation, not about the behaviour of a subclassing mechanism, and thus more belongs here IMO.

Will add dataframe to the test

Copy link
Contributor

Choose a reason for hiding this comment

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

pls just move this, ALL subclass tests are in the same file. This is breaking the pattern for no reason.

s1 = pd.Series([1, 2, 3])
s2 = tm.SubclassedSeries([1, 2, 3])
assert s1.equals(s2)
assert s2.equals(s1)

df1 = pd.DataFrame({"a": [1, 2, 3]})
df2 = tm.SubclassedDataFrame({"a": [1, 2, 3]})
assert df1.equals(df2)
assert df2.equals(df1)

def test_pipe(self):
df = DataFrame({"A": [1, 2, 3]})
f = lambda x, y: x ** y
Expand Down