-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
REGR: Performance of DataFrame axis=1 reduction ops with EA #51955
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
Conversation
Just realized this is separate from #51923. Should this take priority over that? (I think the solution suggested there yesterday is probably the "right" long-term solution) |
@jbrockmendel - I wanted to give this as an option and see what others think. I think the perf regression outweighs the behavior improvement in this case, but I'm also okay sticking with 2.0.x as-is. |
I don't think we should do #51923 for 2.0 even if there was time to squeeze it in. |
I'm generally a believer in "correctness over performance," but this is a cornery enough case im not inclined to argue about it. |
Hah; same. With 2.0rc1 shipped, that pushes me toward leaving 2.0.x as is. I plan on working on improvements for 2.1. Closing. |
@rhshadrach i have tabs open for this and #51923. Am I right in thinking that this is the one you are leaning towards for 2.0 and 51923 should be thought of as an option for 2.1? |
@jbrockmendel - yes, that's correct. |
I'm finding myself going through the diff side-by-side with the diff from #51335 and trying to sort out what is and isnt a reversion. Would it be easier to revert that entirely and then do another PR to un-revert the parts we want to keep? |
@jbrockmendel - sure that sounds good. |
Unfortunately we can't automatically revert that PR because other changes. I can to manually revert, but won't have too much time until Sunday (currently travelling). |
the other pr supersedes this one? |
Release day, moving this to the 2.1 milestone. |
Sorry, my bad, the next release should be 2.0.1, changed it. |
The other PR (#52250) precedes this one, as was created by the request above (#51955 (comment)) |
I don't see any reasonable way to support axis=1 with tzaware dtypes by operating on NumPy arrays - so I think there is no way forward in this direction. |
Ref: #51335 (comment)
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.I think #51923 is the right way to go, but it looks like it will take quite a bit more work. Partial revert of #51335 for axis=1 ops in the meantime.