-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecating Series.argmin and Series.argmax (#16830) #16955
Changes from 12 commits
676bb50
578dbd5
4c770b6
0e039de
db08dda
080fb06
e979c60
e3d3581
b2501b2
7ebf9cd
128f8d4
e106a18
426d8eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -487,11 +487,33 @@ Other API Changes | |
|
||
Deprecations | ||
~~~~~~~~~~~~ | ||
|
||
- :func:`read_excel()` has deprecated ``sheetname`` in favor of ``sheet_name`` for consistency with ``.to_excel()`` (:issue:`10559`). | ||
- ``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`). | ||
- :func:`DataFrame.as_blocks` is deprecated, as this is exposing the internal implementation (:issue:`17302`) | ||
|
||
.. _whatsnew_0210.deprecations.argmin_min | ||
|
||
Series.argmax and Series.argmin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a ref to this subsection. make the title something like
|
||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
- The behavior of :func:`Series.argmax` has been deprecated in favor of :func:`Series.idxmax` (:issue:`16830`) | ||
- The behavior of :func:`Series.argmin` has been deprecated in favor of :func:`Series.idxmin` (:issue:`16830`) | ||
|
||
For compatibility with NumPy arrays, ``pd.Series`` implements ``argmax`` and | ||
``argmin``. Since pandas 0.13.0, ``argmax`` has been an alias for | ||
:meth:`pandas.Series.idxmax`, and ``argmin`` has been an alias for | ||
:meth:`pandas.Series.idxmin`. They return the *label* of the maximum or minimum, | ||
rather than the *position*. | ||
|
||
We've deprecated the current behavior of ``Series.argmax`` and | ||
``Series.argmin``. Using either of these will emit a ``FutureWarning``. Use | ||
:meth:`Series.idxmax` if you want the label of the maximum. Use | ||
``Series.values.argmax()`` if you want the position of the maximum. Likewise for | ||
the minimum. In a future release ``Series.argmax`` and ``Series.argmin`` will | ||
return the position of the maximum or minimum. | ||
|
||
.. _whatsnew_0210.prior_deprecations: | ||
|
||
Removal of prior version deprecations/changes | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1242,16 +1242,31 @@ 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(1, 11) | ||
|
||
s = Series(data) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this was like this before, but it would actually be good to use eg There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you changed it correctly. It's the index of the Series that should be changed, not the actual |
||
with tm.assert_produces_warning(FutureWarning, check_stacklevel=False): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add the issue deprecation issue as a comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make this as a separate function (these deprecation tests) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nothing much, maybe even rename the test a bit to reflect what you are testing test_numpy_argmin_deprecated or somesuch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What he means is just reference the issue number beneath the function definition e.g. "see gh-16830" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh gotcha, thanks for the clarification. I will add that in the next commit. |
||
# The deprecation of Series.argmin also causes a deprecation | ||
# warning when calling np.argmin. This behavior is temporary | ||
# until the implemention of Series.argmin is corrected. | ||
result = np.argmin(s) | ||
|
||
assert result == 1 | ||
|
||
with tm.assert_produces_warning(FutureWarning): | ||
# argmin is aliased to idxmin | ||
result = s.argmin() | ||
|
||
assert result == 1 | ||
|
||
if not _np_version_under1p10: | ||
msg = "the 'out' parameter is not supported" | ||
tm.assert_raises_regex(ValueError, msg, np.argmin, | ||
Series(data), out=data) | ||
with tm.assert_produces_warning(FutureWarning, | ||
check_stacklevel=False): | ||
msg = "the 'out' parameter is not supported" | ||
tm.assert_raises_regex(ValueError, msg, np.argmin, | ||
s, out=data) | ||
|
||
def test_idxmax(self): | ||
# test idxmax | ||
|
@@ -1297,17 +1312,30 @@ def test_idxmax(self): | |
result = s.idxmin() | ||
assert result == 1.1 | ||
|
||
def test_numpy_argmax(self): | ||
def test_numpy_argmax_deprecated(self): | ||
# See gh-16830 | ||
data = np.arange(1, 11) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
s = Series(data) | ||
with tm.assert_produces_warning(FutureWarning, check_stacklevel=False): | ||
# The deprecation of Series.argmax also causes a deprecation | ||
# warning when calling np.argmax. This behavior is temporary | ||
# until the implemention of Series.argmax is corrected. | ||
result = np.argmax(s) | ||
assert result == 10 | ||
|
||
with tm.assert_produces_warning(FutureWarning): | ||
# argmax is aliased to idxmax | ||
result = s.argmax() | ||
|
||
# argmax is aliased to idxmax | ||
data = np.random.randint(0, 11, size=10) | ||
result = np.argmax(Series(data)) | ||
assert result == np.argmax(data) | ||
assert result == 10 | ||
|
||
if not _np_version_under1p10: | ||
msg = "the 'out' parameter is not supported" | ||
tm.assert_raises_regex(ValueError, msg, np.argmax, | ||
Series(data), out=data) | ||
with tm.assert_produces_warning(FutureWarning, | ||
check_stacklevel=False): | ||
msg = "the 'out' parameter is not supported" | ||
tm.assert_raises_regex(ValueError, msg, np.argmax, | ||
s, out=data) | ||
|
||
def test_ptp(self): | ||
N = 1000 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1379,11 +1379,25 @@ def test_numpy_func_call(self): | |
# numpy passes in 'axis=None' or `axis=-1' | ||
funcs = ['sum', 'cumsum', 'var', 'mean', | ||
'prod', 'cumprod', 'std', 'argsort', | ||
'argmin', 'argmax', 'min', 'max'] | ||
'min', 'max'] | ||
for func in funcs: | ||
for series in ('bseries', 'zbseries'): | ||
getattr(np, func)(getattr(self, series)) | ||
|
||
def test_deprecated_numpy_func_call(self): | ||
# NOTE: These should be add to the 'test_numpy_func_call' test above | ||
# once the behavior of argmin/argmax is corrected. | ||
funcs = ['argmin', 'argmax'] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment that we need to include them again in the test above when behaviour of argmin/argmax is changes? (to make sure we don't just delete the test when the deprecation is removed) |
||
for func in funcs: | ||
for series in ('bseries', 'zbseries'): | ||
with tm.assert_produces_warning(FutureWarning, | ||
check_stacklevel=False): | ||
getattr(np, func)(getattr(self, series)) | ||
|
||
with tm.assert_produces_warning(FutureWarning, | ||
check_stacklevel=False): | ||
getattr(getattr(self, series), func)() | ||
|
||
|
||
@pytest.mark.parametrize( | ||
'datetime_type', (np.datetime64, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be in a separate sub-section