PERF: array_equivalent #35256
PERF: array_equivalent #35256
Conversation
| # TODO: pass strict_nane to the .equals methods? | ||
| if isinstance(left, ABCExtensionArray): | ||
| return left.equals(right) | ||
| elif isinstance(right, ABCExtensionArray): | ||
| return right.equals(left) |
There was a problem hiding this comment.
Is this needed? array_equivalent is supposed to operate on ndarrays (and the BlockManager.equals already handles dispatching the EA.equals when needed).
There was a problem hiding this comment.
In the PR that removed Block.equals I thought you agreed that array_equivalent should dispatch for EAs.
I'm OK with removing this check for now; its orthogonal to the perf goal.
|
|
||
| # empty | ||
| if not (np.prod(left.shape) and np.prod(right.shape)): | ||
| return True | ||
| return ((left == right) | (isna(left) & isna(right))).all() |
There was a problem hiding this comment.
Compared to the original fastpath in Block.equals (#34962 (comment)), I think an important factor might also be to use np.isnan here instead of pd.isnan (which should be equivalent for float dtype).
There was a problem hiding this comment.
Hmm, except for things like inf_as_na ... But which we were not using in Block.equals before anyway ..
There was a problem hiding this comment.
This is a good point (two, really). Because of the inf_as_na thing, we should probably consider it as a bugfix and address it separately
| if ldtype == rdtype and ldtype.itemsize != 16: | ||
| ileft = left.view(f"i{ldtype.itemsize}") | ||
| iright = right.view(f"i{rdtype.itemsize}") | ||
| return (ileft == iright).all() |
There was a problem hiding this comment.
Are we sure this is always going to work correctly? It's certainly a minor corner case, but there are ways to get different NaNs, see eg https://stackoverflow.com/a/33371882/653364
There was a problem hiding this comment.
hmm yeah i agree here, nan's would compare not equal, this is eactly why we need the nan mask (you can use np.isnan here though)
There was a problem hiding this comment.
darn, fair point. if we cant compare after int-casting, then i think we lose pretty much all of the perf gain. closing.
Indeed, EA.equals doesn't support that (and not sure it should). But see also my inline comment: is it actually needed to handle EAs here? |
|
maybe merge #35231 first since that PR adds additional tests? |
| if ldtype == rdtype and ldtype.itemsize != 16: | ||
| ileft = left.view(f"i{ldtype.itemsize}") | ||
| iright = right.view(f"i{rdtype.itemsize}") | ||
| return (ileft == iright).all() |
There was a problem hiding this comment.
hmm yeah i agree here, nan's would compare not equal, this is eactly why we need the nan mask (you can use np.isnan here though)
jreback
added
Reshaping
good idea |
xref #35249, this gives about a 3x speedup in the asv reported there
cc @jorisvandenbossche EA.equals doesn't support a strict_nan arg, is there anything we need to do to account for that here?