Deprecating Series.argmin and Series.argmax (#16830) #16955

lphk92 · 2017-07-15T19:43:43Z

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
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.

gfyoung · 2017-07-15T20:07:50Z

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.

gfyoung · 2017-07-15T20:08:13Z

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).

lphk92 · 2017-07-15T20:59:11Z

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.

gfyoung · 2017-07-15T22:26:01Z

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.

gfyoung · 2017-07-15T22:27:01Z

pandas/tests/series/test_analytics.py

-        result = np.argmin(Series(data))
-        assert result == np.argmin(data)
+
+        with pytest.warns(FutureWarning):


Use tm.assert_produces_warning(FutureWarning). We don't use pytest warning context manager.

codecov · 2017-07-15T22:56:55Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2cd85ca). Click here to learn what that means.
The diff coverage is 100%.

@@            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	Coverage Δ
pandas/core/series.py	`94.89% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cd85ca...2b7ebbc. Read the comment docs.

codecov · 2017-07-15T22:56:55Z

Codecov Report

Merging #16955 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            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%> (ø)`	⬆️
#single	`40.32% <80%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/series.py	`94.93% <100%> (ø)`	⬆️
pandas/util/_decorators.py	`78% <100%> (ø)`	⬆️
pandas/io/formats/format.py	`96.07% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.73% <0%> (-0.1%)`	⬇️
pandas/core/generic.py	`92.07% <0%> (ø)`	⬆️
pandas/core/indexes/base.py	`96.29% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0fe5cc...426d8eb. Read the comment docs.

jreback · 2017-07-15T23:55:24Z

looks like #16964 covers this.

jreback · 2017-07-16T01:06:29Z

@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

TomAugspurger · 2017-07-16T01:29:25Z

I think merge this for 0.21 and #16964 for 0.22

gfyoung · 2017-07-16T03:08:07Z

@jreback : The PR description above 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.

gfyoung · 2017-07-16T03:17:16Z

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.

gfyoung · 2017-07-16T03:57:24Z

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.

TomAugspurger · 2017-07-16T12:40:11Z

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.

jreback

lgtm. ping on green.

jreback · 2017-07-16T15:16:00Z

pandas/tests/series/test_analytics.py

-        result = np.argmin(Series(data))
-        assert result == np.argmin(data)
+
+        with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):


add the issue deprecation issue as a comment

make this as a separate function (these deprecation tests)

Since all functions in this test will cause the deprecation warning, if I break them into a separate test, what should be left in here?

nothing much, maybe even rename the test a bit to reflect what you are testing

test_numpy_argmin_deprecated or somesuch

add the issue deprecation issue as a comment

@lphk92 : you should also address this comment from @jreback

@gfyoung I thought that was what I was doing by adding the comments stating that the deprecation warning was also occurring in np.argmax. Could you clarify where you would like a comment added?

What he means is just reference the issue number beneath the function definition e.g. "see gh-16830"

Ahh gotcha, thanks for the clarification. I will add that in the next commit.

jreback · 2017-07-16T15:16:19Z

doc/source/whatsnew/v0.21.0.txt

@@ -116,6 +116,8 @@ Other API Changes
 Deprecations
 ~~~~~~~~~~~~
 - :func:`read_excel()` has deprecated ``sheetname`` in favor of ``sheet_name`` for consistency with ``.to_excel()`` (:issue:`10559`).
+- :method:`Series.argmax` has been deprecated in favor of :method:`Series.idxmax` (:issue:`16830`)


use :func: instead

@jreback Note that in 'theory', :method: is more correct here (as idxmax is a method, not a function), but in practice both work, so it is not that important

jreback · 2017-07-16T15:16:48Z

pandas/tests/series/test_analytics.py

        data = np.random.randint(0, 11, size=10)
-        result = np.argmax(Series(data))
-        assert result == np.argmax(data)
+


jreback · 2017-07-16T15:17:55Z

pandas/tests/series/test_analytics.py

+
+        with tm.assert_produces_warning(FutureWarning):
+            # argmin is aliased to idxmin
+            Series(data).argmin()


add a test on the result as well (and same for argmin)

TomAugspurger · 2017-07-16T15:49:58Z

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.

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 --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?

TomAugspurger · 2017-07-16T15:51:55Z

And then your test should assert that np.argmin(series) does not emit a warning.

jreback · 2017-07-16T16:24:40Z

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

TomAugspurger · 2017-07-16T17:21:52Z

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 ***@***.***> 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.

gfyoung · 2017-07-16T17:27:45Z

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.

TomAugspurger · 2017-07-16T18:16:07Z

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 ***@***.***> 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.

jreback

lgtm. not sure why this is failing. pls rebase.

jreback · 2017-09-23T20:06:53Z

doc/source/whatsnew/v0.21.0.txt

 - ``pd.options.html.border`` has been deprecated in favor of ``pd.options.display.html.border`` (:issue:`15793`).

 - :func:`SeriesGroupBy.nth` has deprecated ``True`` in favor of ``'all'`` for its kwarg ``dropna`` (:issue:`11038`).

+Series.argmax and Series.argmin


add a ref to this subsection. make the title something like

Series.argmin/max are deprecated.

jreback · 2017-09-23T20:07:29Z

doc/source/whatsnew/v0.21.0.txt

+we've deprecated the current behavior of :func:`Series.argmax` and
+:func:`Series.argmin`. Using either of these will emit a ``FutureWarning``.
+
+If you were using ``Series.argmin`` and ``Series.argmax``, please switch to using


I would remove these last 2 paragraphs. keep it short and sweet.

TomAugspurger · 2017-09-24T13:43:55Z

OK, something very strange was going on with the tests. I could only reproduce with python 2.7 when running the sparse tests first:

pytest pandas/tests/sparse/test_series.py::TestSparseSeriesAnalytics::test_deprecated_numpy_func_call  pandas/tests/series/test_analytics.py::TestSeriesAnalytics::test_numpy_argmin_deprecated --tb=short

Running the tests/series/test_analytics.py and then the sparse was fine. Hopefully my last commit makes it work either way.

There may be some warnings to cleanup to. Looking into those now.

TomAugspurger · 2017-09-24T15:34:48Z

Still failed, not sure why. I'm ok with skipping those tests python 2 since

it's working
this will be removed in the next version

jreback · 2017-09-24T19:11:15Z

pandas/tests/series/test_analytics.py

-        data = np.random.randint(0, 11, size=10)
-        result = np.argmin(Series(data))
-        assert result == np.argmin(data)
+    @pytest.mark.skipif(PY2, reason="Buggy assertion on warning")


usually having to do this means that you are NOT catching the actual assertion somewhere else. IOW there is another place where it IS showing the warning, but its not being caught.

jorisvandenbossche · 2017-09-25T07:26:37Z

If you look at the travis log, there are quite some places in other tests that raise this warning, and which probably should be fixed

jorisvandenbossche · 2017-09-25T07:29:13Z

In series/test_operators.py, there are test_assert_argminmax_raises and test_argminmax_with_inf. But should those just catch the warning, or should they be changed to test idxmin/idxmax instead ?

…0-part1

TomAugspurger · 2017-09-26T12:53:09Z

Ok, the tets are (finally) all green. We used argmax internally in https://github.com/pandas-dev/pandas/pull/16955/files#diff-db91df05b120c5eb39d90899b54b5321R348 when we wanted idxmax.

I found one more warning from a test where I failed to switch to idxmax. Merging later today if it's green.

jorisvandenbossche

Looks good!
Just two minor comments

jorisvandenbossche · 2017-09-26T13:10:26Z

pandas/tests/series/test_analytics.py

+            # until the implemention of Series.argmin is corrected.
+            result = np.argmin(s)
+
+        assert result == 0


why are you asserting here directly to 0, and below to the result of expected = s.idxmin(). Both can use the same expected value? (I see in the recent commits you changed this)

I just changed them all to compare directly to scalars. I think that's preferable.

jorisvandenbossche · 2017-09-26T13:11:19Z

pandas/tests/series/test_analytics.py

+        # See gh-16830
+        data = np.arange(10)
+
+        s = Series(data)


I know this was like this before, but it would actually be good to use eg index=np.arange(1, 11), just to have a distinction between positional and label based argmin

I don't think you changed it correctly. It's the index of the Series that should be changed, not the actual data. Because now the tests should fail (the argmin should still be 0 (first location or first index label), while you changed it to assert to 1)

jreback · 2017-09-26T13:15:17Z

pandas/io/formats/format.py

@@ -599,7 +599,7 @@ def to_string(self):
            else:  # max_cols == 0. Try to fit frame to terminal
                text = self.adj.adjoin(1, *strcols).split('\n')
                row_lens = Series(text).apply(len)


shouldn't this just be

max_len = Series(text).str.len().max() ?

Tests seem to pass locally with this.

jreback · 2017-09-26T13:16:01Z

minor comment, otherwise lgtm.

TomAugspurger · 2017-09-26T14:25:25Z

Whoops, fixed.

…

On Tue, Sep 26, 2017 at 9:19 AM, Joris Van den Bossche < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/tests/series/test_analytics.py <#16955 (comment)>: > @@ -1242,16 +1242,32 @@ def test_idxmin(self): result = s.idxmin() assert result == 1 - def test_numpy_argmin(self): - # argmin is aliased to idxmin - data = np.random.randint(0, 11, size=10) - result = np.argmin(Series(data)) - assert result == np.argmin(data) + def test_numpy_argmin_deprecated(self): + # See gh-16830 + data = np.arange(10) + + s = Series(data) I don't think you changed it correctly. It's the index of the Series that should be changed, not the actual data. Because now the tests should fail (the argmin should still be 0 (first location or first index label), while you changed it to assert to 1) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16955 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHItpk13wWbx2s6kXtQ27_UBCozv5Mks5smQgLgaJpZM4OZG7M> .

TomAugspurger · 2017-09-27T15:07:42Z

All green, and no new warnings in the summary. Merging.

Thanks @lphk92!

…s-dev#16955) * Deprecating Series.argmin and Series.argmax (pandas-dev#16830) Added statements about correcting behavior in future commit Add reference to github ticket Fixing placement of github comment Made test code more explicit Fixing unrelated tests that are also throwing warnings Updating whatsnew to give more detail about deprecation Fixing whatsnew and breaking out tests to catch warnings Additional comments and more concise whatsnew Updating deprecate decorator to support custom message DOC: Update docstrings, depr message, and whatsnew * Added debug prints * Try splitting the filters * Reword whatsnew * Change sparse series test * Skip on py2 * Change to idxmin * Remove py2 skips * Catch more warnings * Final switch to idxmax * Consistent tests, refactor to_string * Fixed tests

lphk92 force-pushed the issue-16830-part1 branch from 2784c3f to dfd9d06 Compare July 15, 2017 20:57

gfyoung reviewed Jul 15, 2017

View reviewed changes

gfyoung added 2/3 Compat API Design Deprecate Functionality to remove in pandas and removed 2/3 Compat labels Jul 15, 2017

lphk92 force-pushed the issue-16830-part1 branch from dfd9d06 to 2b7ebbc Compare July 15, 2017 22:56

jreback closed this Jul 15, 2017

TomAugspurger reopened this Jul 16, 2017

jreback requested changes Jul 16, 2017

View reviewed changes

Try splitting the filters

4c770b6

jreback reviewed Sep 23, 2017

View reviewed changes

TomAugspurger added 2 commits September 24, 2017 07:09

Reword whatsnew

0e039de

Change sparse series test

db08dda

Skip on py2

080fb06

jreback reviewed Sep 24, 2017

View reviewed changes

TomAugspurger added 5 commits September 25, 2017 14:56

Change to idxmin

e979c60

Merge remote-tracking branch 'upstream/master' into lphk92-issue-1683…

e3d3581

…0-part1

Remove py2 skips

b2501b2

Catch more warnings

7ebf9cd

Final switch to idxmax

128f8d4

jorisvandenbossche reviewed Sep 26, 2017

View reviewed changes

jreback reviewed Sep 26, 2017

View reviewed changes

jreback approved these changes Sep 26, 2017

View reviewed changes

TomAugspurger added 2 commits September 26, 2017 09:04

Consistent tests, refactor to_string

e106a18

Fixed tests

426d8eb

TomAugspurger merged commit f9d88cd into pandas-dev:master Sep 27, 2017

jorisvandenbossche mentioned this pull request Nov 10, 2017

DOC/DEPR: ensure that @deprecated functions have correct docstring #18215

Merged

jreback mentioned this pull request Dec 7, 2019

DEPR: deprecations log for removed issues #13777

Closed

jbrockmendel mentioned this pull request Dec 11, 2019

API: .argmax should be positional, not an alias for idxmax #16830

Closed

Deprecating Series.argmin and Series.argmax (#16830) #16955

Deprecating Series.argmin and Series.argmax (#16830) #16955

Conversation

lphk92 commented Jul 15, 2017 • edited by gfyoung Loading

gfyoung commented Jul 15, 2017

gfyoung commented Jul 15, 2017

lphk92 commented Jul 15, 2017

gfyoung commented Jul 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2017

Codecov Report

codecov bot commented Jul 15, 2017 • edited Loading

Codecov Report

jreback commented Jul 15, 2017

jreback commented Jul 16, 2017

TomAugspurger commented Jul 16, 2017

gfyoung commented Jul 16, 2017 • edited Loading

gfyoung commented Jul 16, 2017

gfyoung commented Jul 16, 2017

TomAugspurger commented Jul 16, 2017

jreback left a 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

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

Choose a reason for hiding this comment

TomAugspurger commented Jul 16, 2017

TomAugspurger commented Jul 16, 2017

jreback commented Jul 16, 2017

TomAugspurger commented Jul 16, 2017 via email

gfyoung commented Jul 16, 2017

TomAugspurger commented Jul 16, 2017 via email

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Sep 24, 2017

TomAugspurger commented Sep 24, 2017

Choose a reason for hiding this comment

jorisvandenbossche commented Sep 25, 2017

jorisvandenbossche commented Sep 25, 2017

TomAugspurger commented Sep 26, 2017

jorisvandenbossche left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 26, 2017

TomAugspurger commented Sep 26, 2017 via email

TomAugspurger commented Sep 27, 2017

lphk92 commented Jul 15, 2017 •

edited by gfyoung

Loading

codecov bot commented Jul 15, 2017 •

edited

Loading

gfyoung commented Jul 16, 2017 •

edited

Loading