Skip to content

BUG: Fix GroupBy.idxmin/idxmax retrun wrong type on empty #51423

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

Merged
merged 16 commits into from
Feb 25, 2023

Conversation

luke396
Copy link
Contributor

@luke396 luke396 commented Feb 16, 2023

@luke396 luke396 requested a review from jbrockmendel February 17, 2023 04:42
func.__name__ = "idxmax"
return self._idxmin_idxmax(func)

def _idxmin_idxmax(self, func: Callable) -> DataFrame | Series:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if instead of all this you just did return result.astype(self.obj.index.dtype, copy=False)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there are some problems with implementing it directly like this.

If it's better to make minimal changes to the code on the existing basis in this way, I will go ahead and fix these problem on this basis..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can't dtype directly to all result, because there are some types can't be converted to each other, such as inf and NA.

Therefore, I tried to convert only the empty result. Based on the original code, the type is incorrect only when the return value is empty.

@jbrockmendel any suggestion?

@luke396 luke396 force-pushed the idxmin-idxmax-retrun-wrong-type branch from 7854611 to ae26c2e Compare February 18, 2023 07:10
@luke396 luke396 requested review from jbrockmendel and removed request for jbrockmendel February 20, 2023 02:21
@luke396 luke396 marked this pull request as ready for review February 20, 2023 02:21
@luke396 luke396 requested a review from jbrockmendel February 20, 2023 02:21
Copy link
Member

@jbrockmendel jbrockmendel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rhshadrach rhshadrach added this to the 2.1 milestone Feb 25, 2023
@rhshadrach rhshadrach merged commit f943864 into pandas-dev:main Feb 25, 2023
@rhshadrach
Copy link
Member

Thanks @luke396

@luke396 luke396 deleted the idxmin-idxmax-retrun-wrong-type branch February 27, 2023 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: GroupBy.idxmin/idxmax returns wrong dtype on empty Series/DataFrame
4 participants