Skip to content

Fixed bug: Dataframe.sort_values not raising ValueError for ascending-incompatible value and Series.sort_values raising ValueError for int value #42684

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 10 commits into from
Jul 30, 2021
Merged
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.4.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ Indexing
- Bug in :meth:`Series.loc` when with a :class:`MultiIndex` whose first level contains only ``np.nan`` values (:issue:`42055`)
- Bug in indexing on a :class:`Series` or :class:`DataFrame` with a :class:`DatetimeIndex` when passing a string, the return type depended on whether the index was monotonic (:issue:`24892`)
- Bug in indexing on a :class:`MultiIndex` failing to drop scalar levels when the indexer is a tuple containing a datetime-like string (:issue:`42476`)
- Bug in :meth:`DataFrame.sort_values` and :meth:`Series.sort_values` when passing an ascending value, failed to raise or incorrectly raising ``ValueError`` (:issue:`41634`)
-

Missing
Expand Down
3 changes: 2 additions & 1 deletion pandas/core/frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
rewrite_axis_style_signature,
)
from pandas.util._validators import (
validate_ascending,
validate_axis_style_args,
validate_bool_kwarg,
validate_percentile,
Expand Down Expand Up @@ -6190,7 +6191,7 @@ def sort_values( # type: ignore[override]
):
inplace = validate_bool_kwarg(inplace, "inplace")
axis = self._get_axis_number(axis)

ascending = validate_ascending(ascending)
if not isinstance(by, list):
by = [by]
if is_sequence(ascending) and len(by) != len(ascending):
Expand Down
5 changes: 2 additions & 3 deletions pandas/core/series.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
doc,
)
from pandas.util._validators import (
validate_ascending,
validate_bool_kwarg,
validate_percentile,
)
Expand All @@ -69,7 +70,6 @@
)
from pandas.core.dtypes.common import (
ensure_platform_int,
is_bool,
is_dict_like,
is_integer,
is_iterator,
Expand Down Expand Up @@ -3437,8 +3437,7 @@ def sort_values(
)
ascending = ascending[0]

if not is_bool(ascending):
raise ValueError("ascending must be boolean")
ascending = validate_ascending(ascending)

if na_position not in ["first", "last"]:
raise ValueError(f"invalid na_position: {na_position}")
Expand Down
20 changes: 20 additions & 0 deletions pandas/tests/frame/methods/test_sort_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -868,3 +868,23 @@ def test_sort_values_pos_args_deprecation(self):
result = df.sort_values("a", 0)
expected = DataFrame({"a": [1, 2, 3]})
tm.assert_frame_equal(result, expected)

def test_sort_values_validate_ascending_for_value_error(self):
# GH41634
df = DataFrame({"D": [23, 7, 21]})

msg = 'For argument "ascending" expected type bool, received type str.'
with pytest.raises(ValueError, match=msg):
df.sort_values(by="D", ascending="False")

@pytest.mark.parametrize("ascending", [False, 0, 1, True])
def test_sort_values_validate_ascending_functional(self, ascending):
df = DataFrame({"D": [23, 7, 21]})
indexer = df["D"].argsort().values

if not ascending:
indexer = indexer[::-1]

expected = df.loc[df.index[indexer]]
result = df.sort_values(by="D", ascending=ascending)
tm.assert_frame_equal(result, expected)
25 changes: 23 additions & 2 deletions pandas/tests/series/methods/test_sort_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def test_sort_values(self, datetime_series):
expected = ts.sort_values(ascending=False, na_position="first")
tm.assert_series_equal(expected, ordered)

msg = "ascending must be boolean"
msg = 'For argument "ascending" expected type bool, received type NoneType.'
with pytest.raises(ValueError, match=msg):
ts.sort_values(ascending=None)
msg = r"Length of ascending \(0\) must be 1 for Series"
Expand All @@ -63,7 +63,7 @@ def test_sort_values(self, datetime_series):
msg = r"Length of ascending \(2\) must be 1 for Series"
with pytest.raises(ValueError, match=msg):
ts.sort_values(ascending=[False, False])
msg = "ascending must be boolean"
msg = 'For argument "ascending" expected type bool, received type str.'
with pytest.raises(ValueError, match=msg):
ts.sort_values(ascending="foobar")

Expand Down Expand Up @@ -206,6 +206,27 @@ def test_mergesort_decending_stability(self):
expected = Series([3, 2, 1, 1], ["c", "b", "first", "second"])
tm.assert_series_equal(result, expected)

def test_sort_values_validate_ascending_for_value_error(self):
# GH41634
ser = Series([23, 7, 21])

msg = 'For argument "ascending" expected type bool, received type str.'
Copy link
Contributor

Choose a reason for hiding this comment

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

would it not be better to separate the raises ValueError test and the functional tests?
Also, would it not be better to parametrize the functional tests as follows:

@pytest.mark.parametrize("ascending", [False, 0, 1, True])
def test_sort_values_validate_ascending(self, ascending):
    ...
    sorted_ser = ser.sort_values(ascending=ascending)
    ...
    if ascending:
        expected = expected[::-1]
    ...

Copy link
Contributor Author

@gaikwadrahul20 gaikwadrahul20 Jul 25, 2021

Choose a reason for hiding this comment

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

Thanks @attack68. I agree, code will look more clean and short. Pushed a change for this, please check.

Also, I was thinking, does it make more sense to parameterize test_sort_values_validate_ascending_for_value_error test function with msg as a parameter and move code from line 54 to 68 (code attached below) in this test?

        msg = 'For argument "ascending" expected type bool, received type NoneType.'
        with pytest.raises(ValueError, match=msg):
            ts.sort_values(ascending=None)
        msg = r"Length of ascending \(0\) must be 1 for Series"
        with pytest.raises(ValueError, match=msg):
            ts.sort_values(ascending=[])
        msg = r"Length of ascending \(3\) must be 1 for Series"
        with pytest.raises(ValueError, match=msg):
            ts.sort_values(ascending=[1, 2, 3])
        msg = r"Length of ascending \(2\) must be 1 for Series"
        with pytest.raises(ValueError, match=msg):
            ts.sort_values(ascending=[False, False])
        msg = 'For argument "ascending" expected type bool, received type str.'
        with pytest.raises(ValueError, match=msg):
            ts.sort_values(ascending="foobar")

with pytest.raises(ValueError, match=msg):
ser.sort_values(ascending="False")

@pytest.mark.parametrize("ascending", [False, 0, 1, True])
def test_sort_values_validate_ascending_functional(self, ascending):
# GH41634
ser = Series([23, 7, 21])
expected = np.sort(ser.values)

sorted_ser = ser.sort_values(ascending=ascending)
if not ascending:
expected = expected[::-1]

result = sorted_ser.values
tm.assert_numpy_array_equal(result, expected)


class TestSeriesSortingKey:
def test_sort_values_key(self):
Expand Down