Skip to content

BUG: idxmax/min (and argmax/min) for Series with underlying ExtensionArray #37924

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 24 commits into from
Jan 1, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c000668
fix idxmax/min for Series with underlying 'Int' datatype
tonyyyyip Nov 17, 2020
1632081
test added
tonyyyyip Nov 18, 2020
8018586
editted test
tonyyyyip Nov 18, 2020
b076d23
added test for argmax argmin
tonyyyyip Nov 19, 2020
664e4ec
added validations
tonyyyyip Nov 20, 2020
8403c38
The overhaul
tonyyyyip Nov 20, 2020
741c97a
2nd attempt
tonyyyyip Nov 24, 2020
131ae83
2nd attempt
tonyyyyip Nov 24, 2020
3269fb5
simplified idxmaxmin and added parameterised tests
tonyyyyip Nov 25, 2020
0cfb621
fixed newbie mistake
tonyyyyip Nov 26, 2020
d4b13ac
fixed newbie mistake
tonyyyyip Nov 26, 2020
5648eb9
used any_numeric_dtype in test
tonyyyyip Nov 26, 2020
9a1b332
does this solve the pre-commit check failure now?
tonyyyyip Nov 26, 2020
9f5e683
moved EA's skipna logic from Series.argmin/max to EA.argmin/max
tonyyyyip Nov 26, 2020
e73a3d1
moved EA's skipna logic back to Series
tonyyyyip Nov 27, 2020
d78e28c
moved EA's skipna logic back to Series
tonyyyyip Nov 27, 2020
66f3187
added whatsnew entry and extra tests
tonyyyyip Dec 5, 2020
6f01069
moved tests to tests/reductions/rest_reductions.py
tonyyyyip Dec 5, 2020
3540797
added 1.3 whatsnew entry
tonyyyyip Dec 28, 2020
6d0b68e
moved and restructured tests
tonyyyyip Dec 30, 2020
da5bf06
Merge branch 'master' into fix-argmax
tonyyyyip Dec 30, 2020
4f6d111
moved tests to pandas/tests/extensions/methods.py
tonyyyyip Dec 31, 2020
aa909e9
moved tests to pandas/tests/extensions/methods.py
tonyyyyip Dec 31, 2020
dd0ecde
Merge branch 'fix-argmax' of https://github.com/tonyyyyip/pandas into…
tonyyyyip Dec 31, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
simplified idxmaxmin and added parameterised tests
  • Loading branch information
tonyyyyip committed Dec 28, 2020
commit 3269fb5dc519ead5acb91755119a3f520835ed2b
32 changes: 6 additions & 26 deletions pandas/core/series.py
Original file line number Diff line number Diff line change
Expand Up @@ -2076,19 +2076,9 @@ def idxmin(self, axis=0, skipna=True, *args, **kwargs):
>>> s.idxmin(skipna=False)
nan
"""
delegate = self._values
skipna = nv.validate_argmin_with_skipna(skipna, args, kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

no longer validating args/kwargs?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry missed this comment

cc @tonyyyyip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I think if we let idxmax call self.argmax(axis=axis, skipna=skipna, *args, **kwargs) then the args and kwargs can be validated by argmax's validator. I can make another PR if this is desirable.

Copy link
Contributor

Choose a reason for hiding this comment

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

that would be great @tonyyyyip


if isinstance(delegate, ExtensionArray):
if not skipna and isna(delegate).any():
return np.nan
else:
i = delegate.argmin()

else:
i = nanops.nanargmin(delegate, skipna=skipna)
if i == -1:
return np.nan
i = self.argmin(axis=None, skipna=skipna, *args, **kwargs)
if i == -1:
return np.nan
return self.index[i]

def idxmax(self, axis=0, skipna=True, *args, **kwargs):
Expand Down Expand Up @@ -2156,19 +2146,9 @@ def idxmax(self, axis=0, skipna=True, *args, **kwargs):
>>> s.idxmax(skipna=False)
nan
"""
delegate = self._values
skipna = nv.validate_argmax_with_skipna(skipna, args, kwargs)

if isinstance(delegate, ExtensionArray):
if not skipna and isna(delegate).any():
return np.nan
else:
i = delegate.argmax()

else:
i = nanops.nanargmax(delegate, skipna=skipna)
if i == -1:
return np.nan
i = self.argmax(axis=None, skipna=skipna, *args, **kwargs)
if i == -1:
return np.nan
return self.index[i]

def round(self, decimals=0, *args, **kwargs) -> "Series":
Expand Down
65 changes: 47 additions & 18 deletions pandas/tests/series/test_reductions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import numpy as np
import pytest
from datetime import datetime

import pandas as pd
from pandas import MultiIndex, Series
Expand Down Expand Up @@ -116,21 +117,49 @@ def test_validate_stat_keepdims():
np.sum(ser, keepdims=True)


def test_idxmax_idxmin_with_nullable_integer_dtype():
ser = Series([1, 2, 3], dtype="Int64")
assert ser.idxmax() == 2
assert ser.idxmin() == 0


def test_argmax_argmin_with_nullable_integer_dtype():
ser = Series([1, 2, 3], dtype="Int64")
assert ser.argmax() == 2
assert ser.argmin() == 0


def test_with_nullable_float_dtype():
ser = Series([1, 2, 3], dtype="Float64")
assert ser.idxmax() == 2
assert ser.idxmin() == 0
assert ser.argmax() == 2
assert ser.argmin() == 0
@pytest.mark.parametrize('dtype', ['Int64', 'Float64'])
@pytest.mark.parametrize(
"op_name, skipna, expected",
[
('idxmax', True, 'b'),
('idxmin', True, 'a'),
('argmax', True, 1),
('argmin', True, 0),
('idxmax', False, np.nan),
('idxmin', False, np.nan),
('argmax', False, -1),
('argmin', False, -1)
]
)
def test_argminmax_idxminmax(dtype, op_name, skipna, expected):
ser = Series([1, 2, None], index=['a', 'b', 'c'], dtype=dtype)
result = getattr(ser, op_name)(skipna=skipna)
if pd.isna(expected):
assert np.isnan(result)
else:
assert result == expected


@pytest.mark.parametrize(
"op_name, skipna, expected",
[
('idxmax', True, 'b'),
('idxmin', True, 'a'),
('argmax', True, 1),
('argmin', True, 0),
('idxmax', False, np.nan),
('idxmin', False, np.nan),
('argmax', False, -1),
('argmin', False, -1)
]
)
def test_argminmax_idxminmax_with_datetime64_dtype(op_name, skipna, expected):
ser = Series(
[datetime(2020, 1, 1), datetime(2020, 1, 2), None],
index=['a', 'b', 'c']
)
result = getattr(ser, op_name)(skipna=skipna)
if pd.isna(expected):
assert np.isnan(result)
else:
assert result == expected