Repository: pandas-dev/pandas
Pull request: ENH: Add argmax and argmin to ExtensionArray
-  closes #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). 

Problem statement:
Implement min, argmin, max, argmax on ExtensionArrays?
We already have `ExtensionArray.argsort` as part of the EA interface, so we should be able to to do min, argmin, max, and argmax as a composition of `argsort` and `__getitem__`. Do we want to do this?

Hints from issue discussion:
i think this is a nice idea; shows a nice benefit of shared EA infrastructure 
One thing I notice though, our docs on _values_for_argsort don't mention
missing data at all. I think we may need to adjust the API there to return
a tuple of (values, mask), where the mask indicates missing values?

On Fri, Dec 21, 2018 at 8:33 AM Jeff Reback <notifications@github.com>
wrote:

> i think this is a nice idea; shows a nice benefit of shared EA
> infrastructure
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/issues/24382#issuecomment-449403517>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIglNkqMNqDzj8ZLtFDBLjHGe4yT5ks5u7PE-gaJpZM4Zd_jY>
> .
>

When `nan` is invloved, `argsort` behaves in two ways.

The element of index 1 is always `nan` in the following.
1. Putting the `nan` in the beginning

```
In [27]: arr = integer_array([1, np.nan, 2])                                    

In [28]: arr                                                                    
Out[28]: 
<IntegerArray>
[1, NaN, 2]
Length: 3, dtype: Int64

In [29]: arr.argsort()                                                          
Out[29]: array([1, 0, 2])
```

2. Putting the `nan` in the end
```
In [20]: arr1 = integer_array([1, np.nan, 0], dtype='uint8')                    

In [21]: arr1                                                                   
Out[21]: 
<IntegerArray>
[1, NaN, 0]
Length: 3, dtype: UInt8

In [22]: arr1.argsort()                                                         
Out[22]: array([2, 0, 1])

In [23]: idx = pd.Index([1, np.nan, 2])                                         

In [24]: arr = idx.array                                                        

In [25]: arr                                                                    
Out[25]: 
<PandasArray>
[1.0, nan, 2.0]
Length: 3, dtype: float64

In [26]: arr.argsort()                                                          
Out[26]: array([0, 2, 1])
```
Should we standardize where `nan` to be placed? `_values_for_argsort` returns different value for `nan` in different EA.
@makbigc there indeed still some inconsistencies there ... I opened https://github.com/pandas-dev/pandas/issues/21801 for that some time ago. Can you move your comment there?

Commit message: Fix black error

Patch to review:
diff --git a/doc/source/whatsnew/v0.25.1.rst b/doc/source/whatsnew/v0.25.1.rst
index c80195af413f7..8c5596206560f 100644
--- a/doc/source/whatsnew/v0.25.1.rst
+++ b/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`)
 
 .. _whatsnew_0251.enhancements.other:
 
diff --git a/pandas/core/arrays/base.py b/pandas/core/arrays/base.py
index e517be4f03a16..8f8c0f9f46bf1 100644
--- a/pandas/core/arrays/base.py
+++ b/pandas/core/arrays/base.py
@@ -461,6 +461,35 @@ def argsort(self, ascending=True, kind="quicksort", *args, **kwargs):
         result = nargsort(self, kind=kind, ascending=ascending, na_position="last")
         return result
 
+    def argmin(self):
+        """
+        Return the minimun argument indexer.
+
+        Returns
+        -------
+        scalar
+            Minimun argument indexer.
+
+        See Also
+        --------
+        Index.max : Return the maximum value of the object.
+        Series.min : Return the minimum value in a Series.
+        DataFrame.min : Return the minimum values in a DataFrame.
+        """
+        return self.argsort()[0]
+
+    def min(self):
+        min_idx = self.argmin()
+        return self[min_idx]
+
+    def argmax(self):
+        no_nan = self.isna().sum()
+        return self.argsort()[-1 - no_nan]
+
+    def max(self):
+        max_idx = self.argmax()
+        return self[max_idx]
+
     def fillna(self, value=None, method=None, limit=None):
         """
         Fill NA/NaN values using the specified method.
diff --git a/pandas/tests/extension/arrow/arrays.py b/pandas/tests/extension/arrow/arrays.py
index 6a28f76e474cc..1c95fe5b94113 100644
--- a/pandas/tests/extension/arrow/arrays.py
+++ b/pandas/tests/extension/arrow/arrays.py
@@ -155,6 +155,18 @@ def any(self, axis=0, out=None):
     def all(self, axis=0, out=None):
         return self._data.to_pandas().all()
 
+    def argmin(self):
+        raise NotImplementedError
+
+    def min(self):
+        raise NotImplementedError
+
+    def argmax(self):
+        raise NotImplementedError
+
+    def max(self):
+        raise NotImplementedError
+
 
 class ArrowBoolArray(ArrowExtensionArray):
     def __init__(self, values):
diff --git a/pandas/tests/extension/arrow/test_bool.py b/pandas/tests/extension/arrow/test_bool.py
index cc0deca765b41..391788b500d1a 100644
--- a/pandas/tests/extension/arrow/test_bool.py
+++ b/pandas/tests/extension/arrow/test_bool.py
@@ -53,6 +53,16 @@ def test_from_sequence_from_cls(self, data):
 
 
 class TestReduce(base.BaseNoReduceTests):
+    @pytest.mark.parametrize("skipna", [True, False])
+    def test_reduce_series_numeric(self, data, all_numeric_reductions, skipna):
+        op_name = all_numeric_reductions
+        if op_name in ("max", "min"):
+            pass
+        else:
+            ser = pd.Series(data)
+            with pytest.raises(TypeError):
+                getattr(ser, op_name)(skipna=skipna)
+
     def test_reduce_series_boolean(self):
         pass
 
diff --git a/pandas/tests/extension/base/methods.py b/pandas/tests/extension/base/methods.py
index 6d47b0c1d1f77..9d31ae539a64a 100644
--- a/pandas/tests/extension/base/methods.py
+++ b/pandas/tests/extension/base/methods.py
@@ -59,6 +59,30 @@ def test_argsort_missing(self, data_missing_for_sorting):
         expected = pd.Series(np.array([1, -1, 0], dtype=np.int64))
         self.assert_series_equal(result, expected)
 
+    def test_argmax(self, data_missing_for_sorting):
+        # GH 24382
+        result = data_missing_for_sorting.argmax()
+        expected = 0
+        assert result == expected
+
+    def test_max(self, data_missing_for_sorting):
+        # GH 24382
+        result = data_missing_for_sorting.max()
+        expected = data_missing_for_sorting[0]
+        assert result == expected
+
+    def test_argmin(self, data_missing_for_sorting):
+        # GH 24382
+        result = data_missing_for_sorting.argmin()
+        expected = 2
+        assert result == expected
+
+    def test_min(self, data_missing_for_sorting):
+        # GH 24382
+        result = data_missing_for_sorting.min()
+        expected = data_missing_for_sorting[2]
+        assert result == expected
+
     @pytest.mark.parametrize(
         "na_position, expected",
         [
diff --git a/pandas/tests/extension/test_categorical.py b/pandas/tests/extension/test_categorical.py
index f7456d24ad6d3..452242b01b75c 100644
--- a/pandas/tests/extension/test_categorical.py
+++ b/pandas/tests/extension/test_categorical.py
@@ -195,6 +195,14 @@ def test_searchsorted(self, data_for_sorting):
         if not data_for_sorting.ordered:
             raise pytest.skip(reason="searchsorted requires ordered data.")
 
+    def test_max(self):
+        # GH 24382
+        pass
+
+    def test_min(self):
+        # GH 24382
+        pass
+
 
 class TestCasting(base.BaseCastingTests):
     pass
diff --git a/pandas/tests/extension/test_numpy.py b/pandas/tests/extension/test_numpy.py
index 221cf0787d839..7c1261d87160e 100644
--- a/pandas/tests/extension/test_numpy.py
+++ b/pandas/tests/extension/test_numpy.py
@@ -246,6 +246,20 @@ def test_repeat(self, data, repeats, as_series, use_numpy):
         # Fails creating expected
         super().test_repeat(data, repeats, as_series, use_numpy)
 
+    def test_max(self):
+        # GH 24382
+        data = PandasArray(np.array([1, np.nan, 0]))
+        result = data.max()
+        expected = data[0]
+        assert result == expected
+
+    def test_min(self):
+        # GH 24382
+        data = PandasArray(np.array([1, np.nan, 0]))
+        result = data.min()
+        expected = data[2]
+        assert result == expected
+
 
 @skip_nested
 class TestArithmetics(BaseNumPyTests, base.BaseArithmeticOpsTests):


Code review comments:
diff_hunk: @@ -195,6 +195,14 @@ def test_searchsorted(self, data_for_sorting):
         if not data_for_sorting.ordered:
             raise pytest.skip(reason="searchsorted requires ordered data.")
 
+    def test_max(self):
+        # GH 24382
+        pass
+
+    def test_min(self):
+        # GH 24382
+        pass
path: pandas/tests/extension/test_categorical.py
text: @user1:
How come these are empty?

@author:
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`.


@user2:
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.

diff_hunk: @@ -53,6 +53,16 @@ def test_from_sequence_from_cls(self, data):
 
 
 class TestReduce(base.BaseNoReduceTests):
+    @pytest.mark.parametrize("skipna", [True, False])
+    def test_reduce_series_numeric(self, data, all_numeric_reductions, skipna):
+        op_name = all_numeric_reductions
+        if op_name in ("max", "min"):
+            pass
+        else:
+            ser = pd.Series(data)
+            with pytest.raises(TypeError):
path: pandas/tests/extension/arrow/test_bool.py
text: @user1:
Error message?

@author:
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.

@user2:
I think @user1 was asking that you assert something about the error message with the `match=` keyword.

@author:
Those functions just raise the `TypeError` without any error message.