Deprecating Series.argmin and Series.argmax (#16830)
State: closed

 - [ ] closes #xxxx
 - [x] tests added / passed
 - [x] passes ``git diff upstream/master -u -- "*.py" | flake8 --diff``
 - [x] whatsnew entry

This is one step toward closing issue #16830. After this, I will post another pull request containing the code to implement the expected behavior of argmax and argmin for both Series and DataFrame.

Labels:
name: API Design
name: Deprecate
description: Functionality to remove in pandas

Comments:
> After this, I will post another pull request containing the code to implement the expected behavior of argmax and argmin for both Series and DataFrame.

Do note that that PR will not be merged for some time because we need at least one major release of time before we change behavior like that.
Also, you should add tests to make sure that these warnings get issued (we should have tests already for `argmin` and `argmax` in the code-base).
> Also, you should add tests to make sure that these warnings get issued (we should have tests already for argmin and argmax in the code-base).

Done

> Do note that that PR will not be merged for some time because we need at least one major release of time before we change behavior like that.

Yea I figured that would be the case. If it's okay with you I'll still put up the PR so that the code is there and ready to be rebased when the time comes.
> If it's okay with you I'll still put up the PR so that the code is there and ready to be rebased when the time comes.

That works, though it will be up to you primarily to keep it rebased and merge-able in the interim.  We can perhaps make reference to the PR in our deprecations tracker.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@2cd85ca`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/16955/graphs/tree.svg?src=pr&token=eZ4WkYLtcO&width=650&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master   #16955   +/-   ##
=========================================
  Coverage          ?   90.99%           
=========================================
  Files             ?      161           
  Lines             ?    49288           
  Branches          ?        0           
=========================================
  Hits              ?    44849           
  Misses            ?     4439           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.76% <100%> (?)` | |
| #single | `40.2% <100%> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.89% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=footer). Last update [2cd85ca...2b7ebbc](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=h1) Report
> Merging [#16955](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/e0fe5cc60b1dc0d777223bba64b8abfc0e0e02ab?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/16955/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #16955      +/-   ##
==========================================
- Coverage   91.25%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49808    49810       +2     
==========================================
- Hits        45454    45447       -7     
- Misses       4354     4363       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.03% <100%> (ø)` | :arrow_up: |
| #single | `40.32% <80%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.93% <100%> (ø)` | :arrow_up: |
| [pandas/util/\_decorators.py](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvX2RlY29yYXRvcnMucHk=) | `78% <100%> (ø)` | :arrow_up: |
| [pandas/io/formats/format.py](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvZm9ybWF0LnB5) | `96.07% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.73% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `92.07% <0%> (ø)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.29% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=footer). Last update [e0fe5cc...426d8eb](https://codecov.io/gh/pandas-dev/pandas/pull/16955?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

looks like #16964 covers this.
@lphk92 hmm. So the question is to we just to a public change that may break code, or deprecate it for a cycle, and then the change which may break code. In the past what we have done is introduced a new method, e.g. was the case with ``.sort()`` -> ``.sort_values()`` so we side-stepped the issue.

But here we *want* to use ``argmin/max``.

Any thoughts @TomAugspurger @jorisvandenbossche @gfyoung 
I think merge this for 0.21 and #16964 for 0.22
@jreback : The PR description <a href="https://github.com/pandas-dev/pandas/pull/16955#issue-243195924">above</a> outlines @lphk92 's plan of action.  Deprecate for this version and modify in the subsequent one.

I agree with this and therefore am onboard with @TomAugspurger here.
> I think merge this for 0.21 and #16964 for 0.22

Is there going to be a 0.22?  I thought we were going straight to 1.0.
Not sure yet why Appveyor isn't catching the warning, especially the first time around.  Also, this error seems persistent because it occurred twice now.

My first suspicion is that there are other places where `argmin` and `argmax` are called with `Series` that are not being caught (trying searching for the terms via GitHub search in this repository).  I think there might be some based on what I saw.
One thing I didn't really appreciate yesterday, is this will show a warning even for `np.argmin(Series)`, the correct way to do things now. We'll need to sort that out... somehow.
Just to make sure this doesn't get lost, there's an issue with this current approach, as `np.argmin(arr)`, etc. will dispatch to `arr.argmin`, which warns immediately.

```python
In [1]: import pandas as pd; import numpy as np
s = ^[[A
In [2]: s = pd.Series([1, 2])

In [3]: s
Out[3]:
0    1
1    2
dtype: int64

In [4]: np.argmin(s)
/Users/taugspurger/Envs/pandas-dev/lib/python3.6/site-packages/numpy/core/fromnumeric.py:57: FutureWarning: argmin is deprecated. Use idxmin instead
  return getattr(obj, method)(*args, **kwds)
Out[4]: 0
```

These functions *do* take `*args` and `**kwargs`, so we could slip in a `__pandas__deprecated` kwargs, to control whether or not to warn. Something like


```diff
diff --git a/pandas/core/series.py b/pandas/core/series.py
index 5294031be..cfda05486 100644
--- a/pandas/core/series.py
+++ b/pandas/core/series.py
@@ -1287,6 +1287,8 @@ class Series(base.IndexOpsMixin, strings.StringAccessorMixin,
         DataFrame.idxmax
         numpy.ndarray.argmax
         """
+        if '__pandas_deprecated__' in kwargs:
+            warnings.warn(FutureWarning, 'message')
         skipna = nv.validate_argmax_with_skipna(skipna, args, kwargs)
         i = nanops.nanargmax(_values_from_object(self), skipna=skipna)
         if i == -1:
@@ -1294,7 +1296,10 @@ class Series(base.IndexOpsMixin, strings.StringAccessorMixin,
         return self.index[i]
 
     # ndarray compat
-    argmin = deprecate('argmin', idxmin)
+    def argmin(self, axis=None, skipna=True, *args, **kwargs):
+        return self.idxmin(axis=None, skipna=skipna, __pandas__deprecated=True,
+                           *args, **kwargs)
+
     argmax = deprecate('argmax', idxmax)
 
     def round(self, decimals=0, *args, **kwargs):
```

@lphk92 currently this fails in `validate_kwargs` in `pandas/pandas/util/_validators.py`, but you may be able to get it to work. Mind trying it out?
And then your test should assert that `np.argmin(series)` does *not* emit a warning.
@TomAugspurger 

> Just to make sure this doesn't get lost, there's an issue with this current approach, as np.argmin(arr), etc. will dispatch to arr.argmin, which warns immediately.

I don't see a problem with this. I would merge this PR as is.


We don't want to warn when the user is correctly using argmin. If you prefer we could only warn once, with an option to always disable the warning. 

> On Jul 16, 2017, at 11:24, Jeff Reback <notifications@github.com> wrote:
> 
> @TomAugspurger
> 
> Just to make sure this doesn't get lost, there's an issue with this current approach, as np.argmin(arr), etc. will dispatch to arr.argmin, which warns immediately.
> 
> I don't see a problem with this. I would merge this PR as is.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 

> We don't want to warn when the user is correctly using argmin. If you prefer we could only warn once, with an option to always disable the warning.

I agree with @TomAugspurger that we should avoid this.  However, the warning occurs because `np.argmin` calls the `argmin` attribute of the object if that exists.  Thus, the only way to avoid it is to remove the `argmin` method completely...

Perhaps we should update the message to say something about `argmin` being called from `numpy` is okay.  That's the only thing that comes to mind ATM.
My last diff showed how we could cause np.argmin to behave differently than series.argmin. Does it feel too hacky? I don't like it in general, but am ok with it for a single major release. 

> On Jul 16, 2017, at 12:27, gfyoung <notifications@github.com> wrote:
> 
> We don't want to warn when the user is correctly using argmin. If you prefer we could only warn once, with an option to always disable the warning.
> 
> I agree with @TomAugspurger that we should avoid this. However, the warning occurs because np.argmin calls the argmin attribute of the object if that exists. Thus, the only way to avoid it is to remove the argmin method completely...
> 
> Perhaps we should update the message to say something about argmin being called from numpy is okay. That's the only thing that comes to mind ATM.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 

Hmm...yeah it does seem a little hacky.  I'm not sure yet.  @jreback : thoughts?

(Sorry, clicked wrong button)
is there a branch / commit?
Nevermind, I don't think my hack works (I accidentally mixed up argmin and idxmax, causing me to think it worked). Back to the drawing board.
> Nevermind, I don't think my hack works (I accidentally mixed up argmin and idxmax, causing me to think it worked). Back to the drawing board.

@TomAugspurger : Given the difficulty (first starters, accommodating `numpy` calls has been troublesome), should we just merge and then you can follow-up after?

At the very least, I propose that we append a mention about ignoring this if it was called via `numpy`.
I am ok with merging this. @lphk92 couple of nitpicky comments. pls ping on green.
@jreback Thanks. I see your comments and will try to get an update in this evening.
Hello @lphk92! Thanks for updating the PR.















Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on September 26, 2017 at 14:25 Hours UTC
@lphk92 : ping when tests pass, and we'll merge this!
Sigh...Appveyor randomly fails again!  Let me try closing and reopening.
Alright, one more time...then I'll leave it alone for today 😢 
Since Appveyor isn't cooperating, I'll take advantage of this time to make refactoring comments.