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

[jorisvandenbossche]

xref geopandas/geopandas#1420

First, this PR is fixing the "bug" that we shouldn't rely on _constructor being a class that can be used in isinstance (see #32638 for the general discussion about this, this is the only place in our code where _constructor is used like this, AFAIK).
And even if _constructor would be a class, it wouldn't necessarily be the correct class to check with (or not more correct than type(self))

But, so this also brings up the API question: what are the "requirements" we put on the type of other ? Should it be the same type? (as then could also change it to if not type(self) is type(other): ...)
Or is a subclass sufficient (with isinstance) ?
The problem with an isinstance checks with subclasses is that then the order matters (eg subdf.equals(df) would not necessarily give the same answer as df.equals(subdf))

[TomAugspurger]

I vaguely recall having a similar discussion on this recently, but I don't recall the conclusion. Perhaps it was on the PR changing .equals.

Checking that the type matches in .equals is probably safest.

Side-note: we should consider changing DataFrame._constructor to DataFrame.__init__ to ensure this doesn't happen again.

[jorisvandenbossche]

Yes, we recently had the discussion about whether the dtype should be equal -> #33940 (which also touched on the subclasses)

I am fine with exact type checking. I would also be OK with instance check but then in both directions (isinstance(self, type(other)) or isinstance(other, type(self))), so subclasses work in both cases

we should consider changing DataFrame._constructor to DataFrame.init to ensure this doesn't happen again.

That's would actually be fine for me (geopandas is also a good test case ;), we just didn't have any "equals" tests).
I don't think there should be any implications of changing it to DataFrame.__init__?

[jreback]

is there a test that triggers this (so we don't regress)

[jbrockmendel]

makes sense

[jorisvandenbossche]

@jbrockmendel there are multiple options being discussed, so can you be more explicit in what makes sense to you?

[jorisvandenbossche]

For example, do we want exact type equality, or are subclasses fine to be considered equal?

[jbrockmendel]

there are multiple options being discussed, so can you be more explicit in what makes sense to you?

I was referring to the edit in the PR. Haven't formed a strong opinion otherwise.

[jorisvandenbossche]

@jreback since you keep adding back the Reshaping label, what has the equals method to do with Reshaping?
(not that this label is very important, but I don't see the link, so just wondering ;))

[jorisvandenbossche]

Checking that the type matches in .equals is probably safest.

For now, I went with isinstance (but in both directions), as that is actually backwards compatible.
Rigth now series.equals(subclassed_series) passes, so doing an exact type match would break that.
(now I am also fine with seeing that as a bug fix, though, and require the exact type match)

[jreback]

why do u keep taking it off? this doesn’t have any labels and is weird otherwise

[jreback]

needs a whatsnew note, yes?

[jreback]

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

[jorisvandenbossche]

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.

[jreback]

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.

[jorisvandenbossche]

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

[jreback]

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

[jorisvandenbossche]

All good here?

[jreback]

no i asked to move the test quite a while ago

[TomAugspurger]

Fixed the merge conflict.

[jreback]

thanks @jorisvandenbossche