ENH: Add argmax and argmin to ExtensionArray #27801

makbigc · 2019-08-07T14:45:47Z

closes Implement min, argmin, max, argmax on ExtensionArrays? #24382
tests added
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

The methods added in this PR ignore nan ,i.e., skipna=True. The existing categorical.min return nan if categorical contain any nan. This behavior is expected in test_min_max (tests/arrays/categorical/test_analytics.py).

gfyoung · 2019-08-09T20:21:15Z

pandas/tests/extension/arrow/test_bool.py

+            pass
+        else:
+            ser = pd.Series(data)
+            with pytest.raises(TypeError):


Error message?

Thanks for your review.
I think this is non-sense to implement argmax, argmin, max and min for ArrowBoolArray which have two values only. Calling those methods will raise NotImplementedError. Getting but not calling the max and min attributes by gettattr doesn't raise any error.

I think @gfyoung was asking that you assert something about the error message with the match= keyword.

Those functions just raise the TypeError without any error message.

pandas/tests/extension/arrow/test_bool.py

gfyoung · 2019-08-09T20:21:30Z

pandas/tests/extension/test_categorical.py

+
+    def test_min(self):
+        # GH 24382
+        pass


How come these are empty?

The methods added in this PR ignore nan ,i.e., skipna=True. The existing categorical.min return nan if categorical contain any nan. This behavior is expected in test_min_max (tests/arrays/categorical/test_analytics.py).

If min and max of the generic EA ignoring nan is what we want, future PR is required to add skipna parameter to categorical.min and categorical.max.

We don't want a release where these become out of sync, so perhaps a PR implementing skipna=True/False for Categorical first makes sense.

pandas/core/arrays/base.py

TomAugspurger · 2019-08-12T18:49:45Z

pandas/tests/extension/arrow/test_bool.py

+            pass
+        else:
+            ser = pd.Series(data)
+            with pytest.raises(TypeError):


I think @gfyoung was asking that you assert something about the error message with the match= keyword.

pandas/tests/extension/base/methods.py

TomAugspurger · 2019-08-12T18:51:29Z

pandas/tests/extension/test_categorical.py

+
+    def test_min(self):
+        # GH 24382
+        pass


We don't want a release where these become out of sync, so perhaps a PR implementing skipna=True/False for Categorical first makes sense.

jreback · 2019-09-08T16:01:43Z

can you merge master; move the release note to 1.0

makbigc · 2019-09-10T01:34:46Z

@jreback This PR is pending for #27929 in which whether the keyword numeric_only should be deprecated is still in discussion.

jbrockmendel · 2019-10-09T19:46:14Z

@makbigc can you rebase? I think getting these implemented could help with some of the groupby cleanup ive been working on

jbrockmendel · 2019-10-28T18:17:28Z

@makbigc can you rebase

makbigc · 2019-10-29T14:44:52Z

sorry for the late reply. I will work on it.

alimcmaster1 · 2019-11-17T16:32:04Z

doc/source/whatsnew/v0.25.1.rst

@@ -6,6 +6,7 @@ What's new in 0.25.1 (July XX, 2019)
 Enhancements
 ~~~~~~~~~~~~

+- Add :meth:`ExtensionArray.argmax`, :meth:`ExtensionArray.max`, :meth:`ExtensionArray.argmin` and :meth:`ExtensionArray.min` (:issue:`24382`)


Can you move this to 1.0.0 now?

alimcmaster1 · 2019-11-17T16:34:03Z

This would definitely be a neat feature @makbigc are you still wanting to work on this?

alimcmaster1 · 2019-11-30T20:43:55Z

@makbigc can you merge master?

pep8speaks · 2019-12-04T17:01:33Z

Hello @makbigc! 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-07-08 12:15:58 UTC

makbigc · 2019-12-07T13:24:40Z

I don't know what linting error I have made.
Please tell me anything else to change.
Thanks.

alimcmaster1 · 2020-01-03T02:54:12Z

@makbigc mind merging master - will then take a look at any test failures.

MarcoGorelli · 2020-01-17T13:20:53Z

@makbigc sorry to chase you up, just wanted to ask - are you still working on this? Thanks :)

jorisvandenbossche · 2020-05-09T09:54:22Z

OK, I pushed some updates to this PR:

Removed min/max for now, to focus this PR on argmin/argmax (and it's still in the git history of course)
Made an implementation of argmin/argmax based on _values_for_argsort, with missing value handling, similar as we currently have pandas/core/sorting.py::nargsort
Expanded the tests to cover duplicated values / all missing values.

So all please take a new look!

I didn't yet hook this into Series.argmin/argmax (there is the question what the all-NaN case, where Series currently deviates from EA -> #33941).

What's also still missing is a skipna=False keyword, but let's start with reviewing the current updates (-> #33942 for an issue about this)

jbrockmendel · 2020-05-09T14:24:51Z

Made an implementation of argmin/argmax based on _values_for_argsort

My understanding based on other threads is that we shouldn't be using values_for(argsort|factorize) for anything other than (argsort|factorize)

jorisvandenbossche · 2020-05-09T14:43:50Z

@jbrockmendel I think argsort and argmax/min are basically the same regarding requirements on _values_for_argsort ?

Before we better define the expected sematics of _values_for_argsort, we indeed shouldn't use it in other context as argsort (eg indexing). But IMO argsort and argmax/min are the same kind of usage.

jbrockmendel · 2020-05-09T23:01:46Z

Before we better define the expected sematics of _values_for_argsort, we indeed shouldn't use it in other context as argsort (eg indexing). But IMO argsort and argmax/min are the same kind of usage.

I dont understand this distinction. My understanding based on what you've said elsewhere is that we cant count on _values_for_argsort to be implemented, only argsort

jorisvandenbossche · 2020-05-10T08:01:29Z

I dont understand this distinction.

_values_for_argsort is used to implement argsort, and hence has certain requirements on the return value of _values_for_argsort (sort in the same order, but in practice now don't need to handle missing values).
As far as I can see, argmax/argmin have the exact same requirements as argsort (at least I can't think of a case where argsort would work but argmax/argmin would fail or be incorrect. Can you?). But that is not at all the case for using those values in indexing context, or in joining operations, where there are additional requirements (hashable, handle missing values), and thus for those we can't simply assume that _values_for_argsort is usable for those use cases just because it is usable for argsort. But for argmax/argmin, I think we clearly can make this assumption.

My understanding based on what you've said elsewhere is that we cant count on _values_for_argsort to be implemented, only argsort

That was indeed the original idea: as EA author you can either implement _values_for_argsort, or either override EA.argsort with a custom implementation (which indeed means we can't rely on _values_for_argsort being usable directly).
But so, then we can simply expand this implmentation note in the docs to: you can either implement _values_for_argsort (and then argsort, argmin and argmax provide a default implementation based on those values), or implement argsort, argmin and argmax yourself. I don't think that is any problem?

Now, what became apparent in the recent discussions regarding those _values_for_argsort, is that this original idea of "either implement EA._values_for_argsort or either override EA.argsort" in practice isn't followed anymore. We "broke" that since the original EA implementation while fixing some bugs last year, so this is something to be fleshed out -> #27218 (and the last paragraph of #33276 (comment)). But if we can fix this at some point for argsort, we can at once fix it for argmax/argmin as well (as the fix will be the same, I assume). So I dont't think this "broken" part of the EA interface is a reason to not use _values_for_argsort for argmax/argmin.

jorisvandenbossche · 2020-05-12T18:21:35Z

@jbrockmendel any response on the above? Do you understand why / are you OK with using _values_for_argsort here ?

jbrockmendel · 2020-05-12T18:28:14Z

Do you understand why / are you OK with using _values_for_argsort here ?

OK with it

jorisvandenbossche · 2020-05-12T18:30:42Z

And more specific comments on the PR?

jreback · 2020-05-13T01:24:32Z

pandas/core/sorting.py

@@ -319,6 +319,33 @@ def nargsort(
    return indexer


+def nargminmax(values, method: str):


i don’t think these
should be in base.py

rather in array_ops

The reason I put them here is because I think it makes sense to keep it close to nargsort, since the code is very similar (using the same approach with the idx/non_nan_idx etc)

i don’t think these should be in base.py

BTW, this is not base.py, but core/sorting.py, which groups a whole bunch of functionality related to sortable values.
(it might make sense to move sorting.py into the array_ops submodule, but I would do that as a separate move)

k that makes sense (and let’s move sorting.py) as a followon

jbrockmendel · 2020-05-13T23:12:49Z

pandas/core/sorting.py

@@ -319,6 +319,33 @@ def nargsort(
    return indexer


+def nargminmax(values, method: str):


annotate values as EA?

jbrockmendel · 2020-05-13T23:13:52Z

pandas/core/sorting.py

+    func = np.argmax if method == "argmax" else np.argmin
+
+    mask = np.asarray(isna(values))
+    values = values._values_for_argsort()


im not wild about relying on non-public attrs here. could we have the EA method pass values and mask?

_values_for_argsort is a "public developer" API (it's part of the EA interface), that's the entire point of it.

I know we have had the discussion about the point of _values_for_argsort and in principle we could also do without. But at this point, we have that method, it is used for argsort as well, so I think it is most logical that I use it here. And we can continue that general discussion about _values_for_argsort elsewhere.

jbrockmendel · 2020-05-13T23:15:02Z

And more specific comments on the PR?

With the EA methods in place, should we be dispatching to it from Series/DataFrame etc?

jorisvandenbossche · 2020-05-14T06:36:25Z

should we be dispatching to it from Series/DataFrame etc?

Yes, see my "I didn't yet hook this into Series.argmin/argmax" above (#27801 (comment)).

There are some open questions regarding behaviour: #33941 (I suppose we are fine with new extension dtypes to deviate from the current Series behaviour, but not fully sure for longer existing ones (categorical, datetimetz etc)).

TomAugspurger

One question inilne.

Can you add this to the Methods in the ExtensionArray docstring?

pandas/core/sorting.py

jorisvandenbossche · 2020-05-22T15:25:21Z

Any more comments on the current PR?

(I know it still needs to be used in Series.argmin/argmax. I can do that here, but given the API changes ( #33941, #33942), could also do that in a follow-up PR)

jreback

lgtm

jreback · 2020-06-20T16:06:17Z

@makbigc can you merge master

jorisvandenbossche · 2020-07-08T12:16:02Z

Going to merge this once CI passes (after the update with master)

jorisvandenbossche · 2020-07-08T14:38:52Z

@makbigc thanks for starting this PR, and sorry again the review / discussion process didn't go that smoothly initially.

jorisvandenbossche · 2020-07-08T14:47:45Z

Opened #35178 as follow-up issue to actually use this in the Series methods

makbigc added 3 commits August 7, 2019 22:31

Add argmax, max, argmin, min to EA

8303674

Remove argmax, argmin, max, min from ArrowEA

44838f7

Fix black error

8bc9573

gfyoung added the ExtensionArray Extending pandas with custom dtypes or arrays. label Aug 9, 2019

gfyoung reviewed Aug 9, 2019

View reviewed changes

pandas/tests/extension/arrow/test_bool.py Outdated Show resolved Hide resolved

gfyoung reviewed Aug 9, 2019

View reviewed changes

makbigc added 2 commits August 10, 2019 23:13

Add issue number to the test

bc69a4a

update

4787c63

TomAugspurger reviewed Aug 12, 2019

View reviewed changes

makbigc mentioned this pull request Aug 15, 2019

API/DEPR: Change default skipna behaviour + deprecate numeric_only in Categorical.min and max #27929

Merged

alimcmaster1 reviewed Nov 17, 2019

View reviewed changes

makbigc added 6 commits December 2, 2019 21:19

merge for update

8bd82f5

Move the whatsnew entry to v1

e84268b

Add test for categorical

f9c9fea

Add func doc and pre-check for zero length

8b7585f

Add min and max to StringArray

41e8ce4

Add test for empty array

20ca0a2

Resolve black format

1aa7422

simplify test_sparse override

65e1e4c

jreback requested changes May 13, 2020

View reviewed changes

jbrockmendel reviewed May 13, 2020

View reviewed changes

TomAugspurger reviewed May 15, 2020

View reviewed changes

pandas/core/sorting.py Show resolved Hide resolved

jorisvandenbossche mentioned this pull request May 15, 2020

ENH: implement efficient sorting methods for SparseArray (argsort/argmin/argmax) #34197

Closed

Merge remote-tracking branch 'upstream/master' into enh-24382

1b64e2f

jreback approved these changes May 23, 2020

View reviewed changes

jorisvandenbossche added 2 commits June 20, 2020 20:20

Merge remote-tracking branch 'upstream/master' into enh-24382

2cdf16b

Merge remote-tracking branch 'upstream/master' into enh-24382

7c79f5c

jorisvandenbossche merged commit 16bbae0 into pandas-dev:master Jul 8, 2020

jorisvandenbossche mentioned this pull request Jul 8, 2020

ENH: use EA.argmin/argmax in EA._reduce / Series.argmin/argmax #35178

Open

jorisvandenbossche mentioned this pull request Sep 23, 2020

BUG: TypeError on Series(dtype='string').value_counts().idxmax() #36566

Closed

jorisvandenbossche mentioned this pull request Nov 14, 2020

REF: define nanargminmax without values_for_argsort #37815

Merged

pablodz mentioned this pull request May 28, 2021

Series idxmin() TypeError?Need change type to 'float' #41696

Closed

		@@ -319,6 +319,33 @@ def nargsort(
		return indexer


		def nargminmax(values, method: str):

ENH: Add argmax and argmin to ExtensionArray #27801

ENH: Add argmax and argmin to ExtensionArray #27801

Conversation

makbigc commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makbigc Dec 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 8, 2019

makbigc commented Sep 10, 2019

jbrockmendel commented Oct 9, 2019

jbrockmendel commented Oct 28, 2019

makbigc commented Oct 29, 2019

Choose a reason for hiding this comment

alimcmaster1 commented Nov 17, 2019

alimcmaster1 commented Nov 30, 2019

pep8speaks commented Dec 4, 2019 • edited

Comment last updated at 2020-07-08 12:15:58 UTC

makbigc commented Dec 7, 2019

alimcmaster1 commented Jan 3, 2020

MarcoGorelli commented Jan 17, 2020

jorisvandenbossche commented May 9, 2020

jbrockmendel commented May 9, 2020

jorisvandenbossche commented May 9, 2020 • edited

jbrockmendel commented May 9, 2020

jorisvandenbossche commented May 10, 2020

jorisvandenbossche commented May 12, 2020

jbrockmendel commented May 12, 2020

jorisvandenbossche commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented May 13, 2020

jorisvandenbossche commented May 14, 2020

TomAugspurger left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented May 22, 2020

jreback left a comment

Choose a reason for hiding this comment

jreback commented Jun 20, 2020

jorisvandenbossche commented Jul 8, 2020

jorisvandenbossche commented Jul 8, 2020

jorisvandenbossche commented Jul 8, 2020

makbigc Dec 5, 2019 •

edited

pep8speaks commented Dec 4, 2019 •

edited

jorisvandenbossche commented May 9, 2020 •

edited