Skip to content
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: Revert GH51335 #52250

Closed
wants to merge 5 commits into from
Closed

Conversation

rhshadrach
Copy link
Member

Ref: #51955 (comment)

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@jbrockmendel - this only reverts the change to _reduce, but leaves other changes untouched. Would you prefer to revert all of #51335?

@rhshadrach rhshadrach added the Reduction Operations sum, mean, min, max, etc. label Mar 27, 2023
@jbrockmendel
Copy link
Member

Does the whatsnew from #51335 need to be changed/reverted?

@rhshadrach
Copy link
Member Author

@jbrockmendel - yes, it did - thanks.

@jbrockmendel
Copy link
Member

OK this looks fine. Let's reiterate the plan for when i inevitably forget: merge this, backport to 2.0, then for 2.1 do either #51923, which might need to be updated to un-revert some or all of this?

@rhshadrach
Copy link
Member Author

I think you meant #51955 instead of #51923. #51955 can either go into 2.0 or 2.1 depending on whether there is time.

@phofl
Copy link
Member

phofl commented Apr 2, 2023

Is this ready for 2.0?

@rhshadrach
Copy link
Member Author

I believe this is ready, wanted to make sure @jbrockmendel is on board with the plan. I think both this and #51955 would be good for 2.0.1.

@rhshadrach rhshadrach added this to the 2.0.1 milestone Apr 3, 2023
@rhshadrach rhshadrach added the Regression Functionality that used to work in a prior pandas version label Apr 3, 2023
@rhshadrach
Copy link
Member Author

Ack - need to update the whatsnew, so no, this isn't ready anymore.

@rhshadrach rhshadrach changed the title BUG: Revert GH51335 REGR: Revert GH51335 Apr 3, 2023
@jbrockmendel
Copy link
Member

Yes I am on board with the plan.

@rhshadrach
Copy link
Member Author

I'm a bit more negative on this now. With 2.0.0 now released, this with #51955 will now reintroduce bad behavior (test_minmax_tzaware_skipna_axis_1 is xfailed). I'm going to see if I can get that test working in #51955, and if this is the case then I think we can go foward.

Without being able to fix that test, I think this path should be abandoned and we should instead pursue (a) increase perf of transpose as @jorisvandenbossche has suggested (#52083) and (b) use online ops where possible as @jbrockmendel has suggested (#52083 (comment)).

@jbrockmendel
Copy link
Member

I like this plan; I'm excited to be a part of it.

@rhshadrach
Copy link
Member Author

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.

@rhshadrach rhshadrach closed this Apr 8, 2023
@rhshadrach rhshadrach deleted the revert_51335 branch April 16, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reduction Operations sum, mean, min, max, etc. Regression Functionality that used to work in a prior pandas version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants