Skip to content

Deprecate passing args as positional in sort_values #41505

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

Merged
Merged
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.3.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,7 @@ Deprecations
- Deprecated setting :attr:`Categorical._codes`, create a new :class:`Categorical` with the desired codes instead (:issue:`40606`)
- Deprecated behavior of :meth:`DatetimeIndex.union` with mixed timezones; in a future version both will be cast to UTC instead of object dtype (:issue:`39328`)
- Deprecated using ``usecols`` with out of bounds indices for ``read_csv`` with ``engine="c"`` (:issue:`25623`)
- Deprecated passing arguments as positional (except for ``by``) in :meth:`DataFrame.sort_values` and :meth:`Series.sort_values` (:issue:`41485`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change won't solve the error: Signature of "sort_values" incompatible with supertype "NDFrame" [override]. maybe we could also give by a default value (the dataframe columns). The 'by' keyword could still be passed positionally if it has a default value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also perhaps deprecate the axis keyword on Series at the same time to clean this up further.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also perhaps deprecate the axis keyword on Series at the same time to clean this up further.

I don't understand, wouldn't this be incompatible with the supertype?

maybe we could also give by a default value (the dataframe columns). The 'by' keyword could still be passed positionally if it has a default value.

Sure, could that - that could be done directly in 1.3 without a prior warning, right? As current behaviour would be preserved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could also give by a default value (the dataframe columns). The 'by' keyword could still be passed positionally if it has a default value.

That might be a good idea, but I would personally leave that for a separate PR (as it seems an independent new feature)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Deprecated passing arguments as positional (except for ``by``) in :meth:`DataFrame.sort_values` and :meth:`Series.sort_values` (:issue:`41485`)
- Deprecated passing arguments as positional in :meth:`DataFrame.sort_values` (except for ``by``) and :meth:`Series.sort_values` (:issue:`41485`)


.. ---------------------------------------------------------------------------

Expand Down
2 changes: 2 additions & 0 deletions pandas/core/frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
Appender,
Substitution,
deprecate_kwarg,
deprecate_nonkeyword_arguments,
doc,
rewrite_axis_style_signature,
)
Expand Down Expand Up @@ -6188,6 +6189,7 @@ def f(vals) -> tuple[np.ndarray, int]:
# ----------------------------------------------------------------------
# Sorting
# TODO: Just move the sort_values doc here.
@deprecate_nonkeyword_arguments(version="2.0", allowed_args=["self", "by"])
@Substitution(**_shared_doc_kwargs)
@Appender(NDFrame.sort_values.__doc__)
# error: Signature of "sort_values" incompatible with supertype "NDFrame"
Expand Down
2 changes: 2 additions & 0 deletions pandas/core/series.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
from pandas.util._decorators import (
Appender,
Substitution,
deprecate_nonkeyword_arguments,
doc,
)
from pandas.util._validators import (
Expand Down Expand Up @@ -3224,6 +3225,7 @@ def update(self, other) -> None:
# ----------------------------------------------------------------------
# Reindexing, sorting

@deprecate_nonkeyword_arguments(version="2.0", allowed_args=["self"])
def sort_values(
self,
axis=0,
Expand Down
10 changes: 10 additions & 0 deletions pandas/tests/frame/methods/test_sort_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -856,3 +856,13 @@ def test_sort_column_level_and_index_label(
tm.assert_frame_equal(result, expected)
else:
tm.assert_frame_equal(result, expected)

def test_sort_values_pos_args_deprecation(self):
# https://github.com/pandas-dev/pandas/issues/41485
df = DataFrame({"a": [1, 2, 3]})
msg = (
r"Starting with Pandas version 2\.0 all arguments of sort_values except "
r"for the arguments 'self' and 'by' will be keyword-only"
)
with tm.assert_produces_warning(FutureWarning, match=msg):
df.sort_values("a", 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as in other PRs regarding asserting the result

20 changes: 15 additions & 5 deletions pandas/tests/series/methods/test_sort_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -187,30 +187,40 @@ def test_sort_values_ignore_index(
tm.assert_series_equal(result_ser, expected)
tm.assert_series_equal(ser, Series(original_list))

def test_sort_values_pos_args_deprecation(self):
# https://github.com/pandas-dev/pandas/issues/41485
ser = Series([1, 2, 3])
msg = (
r"Starting with Pandas version 2\.0 all arguments of sort_values except "
r"for the argument 'self' will be keyword-only"
)
with tm.assert_produces_warning(FutureWarning, match=msg):
ser.sort_values(0)


class TestSeriesSortingKey:
def test_sort_values_key(self):
series = Series(np.array(["Hello", "goodbye"]))

result = series.sort_values(0)
result = series.sort_values(axis=0)
expected = series
tm.assert_series_equal(result, expected)

result = series.sort_values(0, key=lambda x: x.str.lower())
result = series.sort_values(axis=0, key=lambda x: x.str.lower())
expected = series[::-1]
tm.assert_series_equal(result, expected)

def test_sort_values_key_nan(self):
series = Series(np.array([0, 5, np.nan, 3, 2, np.nan]))

result = series.sort_values(0)
result = series.sort_values(axis=0)
expected = series.iloc[[0, 4, 3, 1, 2, 5]]
tm.assert_series_equal(result, expected)

result = series.sort_values(0, key=lambda x: x + 5)
result = series.sort_values(axis=0, key=lambda x: x + 5)
expected = series.iloc[[0, 4, 3, 1, 2, 5]]
tm.assert_series_equal(result, expected)

result = series.sort_values(0, key=lambda x: -x, ascending=False)
result = series.sort_values(axis=0, key=lambda x: -x, ascending=False)
expected = series.iloc[[0, 4, 3, 1, 2, 5]]
tm.assert_series_equal(result, expected)