Repository: pandas-dev/pandas
Pull request: Fixed bug: Dataframe.sort_values not raising ValueError for ascending-incompatible value and Series.sort_values raising ValueError for int value
- [x] closes #41634
- [x] tests added / passed
- [x] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#code-standards) for how to run them
- [x] whatsnew entry


Problem statement:
ENH: Raise error on ascending='False'
#### Is your feature request related to a problem?

when sort_values ascending parameter is set to a string (df.sort_values(col_name, ascending='False') it does not error although it's meaningless, this could be quite confusing.

#### Describe the solution you'd like

Raise an error if ascending is equal to values that are not boolean 

#### API breaking implications

Error raising

#### Describe alternatives you've considered

Warning is also good, but I think error makes more sense

#### Additional context

for an hour I was wondering why ascending='False'
 is not working

```python
isinstance('False', bool) :)

```

Hints from issue discussion:
Hello, I am a new to open source contribution. Can I take up this enhancement?
You can't be newer than me :)
If it's up to me, you have my blessings !

Ezer

On Mon, May 24, 2021 at 1:15 PM Vishal Kumar Prasad <
***@***.***> wrote:

> Hello, I am a new to open source contribution. Can I take up this
> enhancement?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/issues/41634#issuecomment-846940277>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACPNI5NADVRFMFGDFOLOSMDTPIRL7ANCNFSM45L7FWRQ>
> .
>

Hi @ezerkar,
This seems reasonable to me, since we do this for ``sort_index`` as well. (see #39451)
@vishalkumarprasad Thanks for volunteering to contribute to pandas. Feel free to take up this issue. You can find our contributing guidelines here. https://pandas.pydata.org/docs/development/contributing.html and if you need help, don't hesitate to ping me :). 
take
Thanks to you all

בתאריך יום ב׳, 24 במאי 2021, 20:07, מאת Vishal Kumar Prasad ‏<
***@***.***>:

> take
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/issues/41634#issuecomment-847199445>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACPNI5K7K57KOUPOT2QXDNLTPKBW7ANCNFSM45L7FWRQ>
> .
>

You can use sorted() function here.
Code:
```
a = ("h", "b", "a", "c", "f", "d", "e", "g")
x = sorted(a)
print(x)
```
Output:
`['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h']`

Hi Eric, I am already working on this enhancement.
Hey, is this issue still open or taken?
AFAIK it's taken

בתאריך יום ה׳, 27 במאי 2021, 18:07, מאת DaPy15 ***@***.***>:

> Hey, is this issue still open or taken?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/issues/41634#issuecomment-849712963>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACPNI5K2T25A4H73SM7PXDDTPZN4DANCNFSM45L7FWRQ>
> .
>

Hi all, while working on this problem, i figured another issue where if you put series.sort_values ascending parameter to 1 or 0, it fails. However according to the code, it shouldn't fail.
Can someone confirm?

Commit message: DOC: Corrected line in rst file

Patch to review:
diff --git a/doc/source/whatsnew/v1.4.0.rst b/doc/source/whatsnew/v1.4.0.rst
index 8d96d49daba4f..bccc65a34d6a7 100644
--- a/doc/source/whatsnew/v1.4.0.rst
+++ b/doc/source/whatsnew/v1.4.0.rst
@@ -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
 ^^^^^^^
diff --git a/pandas/core/frame.py b/pandas/core/frame.py
index 8b82021375a28..ef4a94164216e 100644
--- a/pandas/core/frame.py
+++ b/pandas/core/frame.py
@@ -85,6 +85,7 @@
     rewrite_axis_style_signature,
 )
 from pandas.util._validators import (
+    validate_ascending,
     validate_axis_style_args,
     validate_bool_kwarg,
     validate_percentile,
@@ -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):
diff --git a/pandas/core/series.py b/pandas/core/series.py
index e61ce8e74629b..2f64a9b612095 100644
--- a/pandas/core/series.py
+++ b/pandas/core/series.py
@@ -57,6 +57,7 @@
     doc,
 )
 from pandas.util._validators import (
+    validate_ascending,
     validate_bool_kwarg,
     validate_percentile,
 )
@@ -69,7 +70,6 @@
 )
 from pandas.core.dtypes.common import (
     ensure_platform_int,
-    is_bool,
     is_dict_like,
     is_integer,
     is_iterator,
@@ -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}")
diff --git a/pandas/tests/frame/methods/test_sort_values.py b/pandas/tests/frame/methods/test_sort_values.py
index d46796bcd978b..76d675ee21007 100644
--- a/pandas/tests/frame/methods/test_sort_values.py
+++ b/pandas/tests/frame/methods/test_sort_values.py
@@ -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)
diff --git a/pandas/tests/series/methods/test_sort_values.py b/pandas/tests/series/methods/test_sort_values.py
index 67f986c0949ca..3b96c380ffd89 100644
--- a/pandas/tests/series/methods/test_sort_values.py
+++ b/pandas/tests/series/methods/test_sort_values.py
@@ -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"
@@ -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")
 
@@ -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.'
+        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):


Code review comments:
diff_hunk: @@ -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`)
path: doc/source/whatsnew/v1.4.0.rst
text: @user1:
The first part is equal to the DataFrame comment? You can compress both lines then like in 225