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

xref https://github.com/geopandas/geopandas/issues/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 https://github.com/pandas-dev/pandas/issues/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)`)



Labels:
name: API Design

Comments:
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.
Yes, we recently had the discussion about whether the *dtype* should be equal -> https://github.com/pandas-dev/pandas/issues/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__`?


is there a test that triggers this (so we don't regress)
makes sense
@jbrockmendel there are multiple options being discussed, so can you be more explicit in what makes sense to you?
For example, do we want exact type equality, or are subclasses fine to be considered equal?
> 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.
@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 ;))
> 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)
why do u keep taking it off? this doesn’t have any labels and is weird otherwise
All good here?
no i asked to move the test quite a while ago 
Fixed the merge conflict.
thanks @jorisvandenbossche