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
3 changes: 2 additions & 1 deletion doc/source/whatsnew/v1.4.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,8 @@ 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` when passing a ascending-incompatible value, failed to raise ``ValueError`` (:issue:`41634`)
- Bug in :meth:`Series.sort_values` when passing a ascending-compatible value; int, raising ``ascending must be boolean`` error (: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
22 changes: 22 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,25 @@ 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(self):
# GH41634
df = DataFrame({"D": [23, 7, 21]})
indexer = df["D"].argsort().values
r_indexer = indexer[::-1]

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

expected_df = df.loc[df.index[r_indexer]]
sorted_df = df.sort_values(by="D", ascending=False)
tm.assert_frame_equal(sorted_df, expected_df)

expected_df = df.loc[df.index[r_indexer]]
sorted_df = df.sort_values(by="D", ascending=0)
tm.assert_frame_equal(sorted_df, expected_df)

expected_df = df.loc[df.index[indexer]]
sorted_df = df.sort_values(by="D", ascending=1)
tm.assert_frame_equal(sorted_df, expected_df)
23 changes: 21 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,25 @@ 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(self):
# GH41634
ser = Series([23, 7, 21])
expected = np.sort(ser.values)[::-1]

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

sorted_ser = ser.sort_values(ascending=False)
tm.assert_numpy_array_equal(sorted_ser.values, expected)

sorted_ser = ser.sort_values(ascending=0)
tm.assert_numpy_array_equal(sorted_ser.values, expected)

expected = expected[::-1]
sorted_ser = ser.sort_values(ascending=1)
tm.assert_numpy_array_equal(sorted_ser.values, expected)


class TestSeriesSortingKey:
def test_sort_values_key(self):
Expand Down