-
-
Notifications
You must be signed in to change notification settings - Fork 18.8k
BUG: groupby.idxmin/idxmax with all NA values should raise #62026
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
BUG: groupby.idxmin/idxmax with all NA values should raise #62026
Conversation
doc/source/whatsnew/v3.0.0.rst
Outdated
@@ -504,7 +504,7 @@ Renamed the following offset aliases (:issue:`57986`): | |||
|
|||
Other Removals | |||
^^^^^^^^^^^^^^ | |||
- :class:`.DataFrameGroupBy.idxmin`, :class:`.DataFrameGroupBy.idxmax`, :class:`.SeriesGroupBy.idxmin`, and :class:`.SeriesGroupBy.idxmax` will now raise a ``ValueError`` when used with ``skipna=False`` and an NA value is encountered (:issue:`10694`) | |||
- :class:`.DataFrameGroupBy.idxmin`, :class:`.DataFrameGroupBy.idxmax`, :class:`.SeriesGroupBy.idxmin`, and :class:`.SeriesGroupBy.idxmax` will now raise a ``ValueError`` when a group has all NA values, or when used with ``skipna=False`` and any NA value is encountered (:issue:`10694`) |
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.
does 10694 cover both of these cases?
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.
No - added the linked issue in the OP. Thanks!
@@ -283,8 +283,6 @@ def test_idxmin_idxmax_extremes_skipna(skipna, how, float_numpy_dtype): | |||
np.nan, | |||
max_value, | |||
np.nan, | |||
np.nan, | |||
np.nan, |
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.
this change is bc there's an all-NA group so itll now raise?
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.
Yes, and the case of testing for raising is done elsewhere.
Couple of small questions, assuming no surprises on those: LGTM |
…groupby_idxmin_all_na
Thanks @rhshadrach |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.