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

[tonyyyyip]

• closes BUG: regression in error raised by idxmin/idxmax for extension dtypes #32749, closes BUG: idxmin() fails for nullable integer data type (Int64) #33719, closes BUG: TypeError on Series(dtype='string').value_counts().idxmax() #36566
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

Currently pd.Series([1, 2, 3]).astype('Int8').idxmax() raises an error.
This request proposes a fix.
My first time using Github and opening a pull request. Apologise if it's no good.

[arw2019]

Thanks @tonyyyyip for the PR (and welcome to pandas!)

A few things to get this on the road:

• is there are an issue in the tracker related to this? If no, let's open one.
• Link the issue here (just mention #....)
• Can you add tests to illustrate the new behavior? They should fail on master and pass on this branch. Tests are typically the first thing we look at when reviewing

Ping once you've got the tests up and we'll take another look. Don't hesistate to post any questions you might have at any point!

FYI We have more detailed info on opening PRs in the contributors' guide in case that's helpful now or later on

[pep8speaks]

Hello @tonyyyyip! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-12-31 01:56:35 UTC

[tonyyyyip]

Thanks @arw2019!

This should solve #33719. Currently the code sample raises TypeError: argmax() takes 1 positional argument but 2 were given. This is because the implementation of argmax/min of ExtensionArray takes only 1 argument self but the "interface" passes an additional argument None to it.

Added test. Not sure if I have done right though. Thanks again.

[arw2019]

some comments on the test, lmk if you have any questions

[arw2019]

You also wanna look at the CI checks (in my experience easiest thing is to use the pre-commit tool locally)

[tonyyyyip]

Thanks @arw2019. I am completely new to testing. I will check that out.

[jorisvandenbossche]

This is possibly also related to #35178 (although that speaks about argmin/argmax, and not idxmin/idxmax)

[tonyyyyip]

This is possibly also related to #35178 (although that speaks about argmin/argmax, and not idxmin/idxmax)

Yes both the argmax and idxmax Series methods call the argmax method of the underlying _values with the additional argument None (for the axis), which is normally fine when the _values is an nparray, but breaks when it's an EA, because the current EA implementation of argmax does not take any additional arguments. This PR fixes that.

[arw2019]

This is possibly also related to #35178 (although that speaks about argmin/argmax, and not idxmin/idxmax)

Yes both the argmax and idxmax Series methods call the argmax method of the underlying _values with the additional argument None (for the axis), which is normally fine when the _values is an nparray, but breaks when it's an EA, because the current EA implementation of argmax does not take any additional arguments. This PR fixes that.

Can you add tests for argmax/argmin so we can decide if this resolves #35178 (if yes great!)

[jorisvandenbossche]

Thanks for the update!

[tonyyyyip]

Gentle ping to see if this is going anywhere.

Please let me know if my changes are not what what the requests wanted, as I might have misunderstood the requests.

Also I understand it was decided that argmax and idxmax should be separated because argmax is likely understated. I am happy to follow that philosophy here.

[arw2019]

@tonyyyyip I think the way forward here is to first get consensus on #33942 regarding the skipna issue so you know what behavior to implement.

Then we can decide where the code goes (into the Series impl or elsewhere) and bring this across the finish line

[jreback]

I think this is fine right as it is. If we decide to change #33942 it would just move logic here inside the EA, no external change. meanwhile this fixes the bug.

pls add a whatsnew note
does this have a reference issue?

[tonyyyyip]

@jreback Added whatsnew entry and referenced issues. Thanks.

[jorisvandenbossche]

Some small comments on the tests

[jreback]

thanks @tonyyyyip very nice

[jbrockmendel]

no longer validating args/kwargs?

[jreback]

sorry missed this comment

cc @tonyyyyip

[tonyyyyip]

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.

[jreback]

that would be great @tonyyyyip

[tonyyyyip]

thanks @tonyyyyip very nice

Thanks.