-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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.
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
REGR: Performance regression in axis=1 DataFrame ops #51923
Conversation
axis = self._get_axis_number(axis) | ||
assert axis in [0, 1] | ||
|
||
if len(df._mgr) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be len(df.columns)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also we only need to care about common_dtype in axis=1 case right? can we avoid these checks otherwise?
Yikes. I'm leaning towards just acknowledging that these ops with MaskedDtype will always perform poorly. |
Given the current state of this PR I don't disagree, but it can be somewhat cleaned up. I'm curious if it's the logic after the NumPy computation (L10538-10557) that gives you that reaction, as I don't see anyway around that part. I'm thinking this is worth it as it fixes not only performance but also some dtype issues:
That said, if this doesn't look viable for the masked dtypes, perhaps some thought could go into changing the EA API to better support axis=1 ops (even for third party EAs)? |
return out | ||
|
||
if is_np or not is_numeric_dtype(common_dtype): | ||
values = df.values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really relevant here but in general df._values is better than df.values, preserves e.g. DatetimeArray[tzaware]
Thanks for pointing this out, I was working under the wrong impression. For now I'm going to ignore the perf issue (as mentioned, I'm basically willing to write it off) and only focus on the bugs (which I expect includes #42895). IIUC the crux of the problem is in Block.reduce where we do
In the ndarray reductions have a Shorter term what if we tried a kludgy patch in Block.reduce checking for MaskedEADtype? [Obligatory mention that 2D EAs would have prevented this and so so many other issues.] |
The
though i think that only works for a subset of cases |
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
Ref: #51335 (comment)
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.This is a work in progress; if this looks like a viable way forward it at least needs tests (and maybe fixes) for non-numeric dtypes, a whatsnew, and some cleanups. This PR gets around the performance regression by operating on NumPy dtypes only in cases where we can be certain the op can correctly be computed (namely NumPy and masked EA dtypes).
On 1.5.x, the axis=1 would result in NumPy float dtype, even when e.g. summing all integers. Here the result remains a masked EA and ops like sum, prod don't upcast to float.
cc @jbrockmendel @jorisvandenbossche