From 3677a9e6f4c2932a10a9d5e4539205ca704cef36 Mon Sep 17 00:00:00 2001 From: Rahul Date: Fri, 23 Jul 2021 15:50:44 +0530 Subject: [PATCH 1/7] BUG: Dataframe.sort_values not raising ValueError for ascending-incompatible value (#41634) --- doc/source/whatsnew/v1.4.0.rst | 3 ++- pandas/core/frame.py | 3 ++- .../tests/frame/methods/test_sort_values.py | 22 +++++++++++++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/doc/source/whatsnew/v1.4.0.rst b/doc/source/whatsnew/v1.4.0.rst index 8d96d49daba4f..8f6ef215dbb12 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`) + 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/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) From a84e1c0aaf4ebb609434aab165d3e22eb4154519 Mon Sep 17 00:00:00 2001 From: Rahul Date: Fri, 23 Jul 2021 16:03:25 +0530 Subject: [PATCH 2/7] BUG: Series.sort_values not raising ValueError for ascending-incompatible value (#41634) --- doc/source/whatsnew/v1.4.0.rst | 2 +- pandas/core/series.py | 5 ++-- .../tests/series/methods/test_sort_values.py | 23 +++++++++++++++++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/doc/source/whatsnew/v1.4.0.rst b/doc/source/whatsnew/v1.4.0.rst index 8f6ef215dbb12..d53937f98e3ba 100644 --- a/doc/source/whatsnew/v1.4.0.rst +++ b/doc/source/whatsnew/v1.4.0.rst @@ -225,7 +225,7 @@ Indexing - 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-incompatible value, failed to raise ``ValueError`` (:issue:`41634`) Missing ^^^^^^^ 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/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): From f50c032a03f5108132be35e7a10712cf5e19ff61 Mon Sep 17 00:00:00 2001 From: Rahul Date: Fri, 23 Jul 2021 16:23:32 +0530 Subject: [PATCH 3/7] DOC: Corrected line in rst file --- doc/source/whatsnew/v1.4.0.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/source/whatsnew/v1.4.0.rst b/doc/source/whatsnew/v1.4.0.rst index d53937f98e3ba..bccc65a34d6a7 100644 --- a/doc/source/whatsnew/v1.4.0.rst +++ b/doc/source/whatsnew/v1.4.0.rst @@ -225,7 +225,7 @@ Indexing - 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-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 ^^^^^^^ From 0ed2a0150b5be68a08d8cd9e4f6e370ab4191221 Mon Sep 17 00:00:00 2001 From: Rahul Date: Sat, 24 Jul 2021 19:14:08 +0530 Subject: [PATCH 4/7] Changed doc statements and object values as per review --- doc/source/whatsnew/v1.4.0.rst | 4 ++-- pandas/tests/series/methods/test_sort_values.py | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/doc/source/whatsnew/v1.4.0.rst b/doc/source/whatsnew/v1.4.0.rst index bccc65a34d6a7..b4efe83059538 100644 --- a/doc/source/whatsnew/v1.4.0.rst +++ b/doc/source/whatsnew/v1.4.0.rst @@ -224,8 +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`) +- 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 ^^^^^^^ diff --git a/pandas/tests/series/methods/test_sort_values.py b/pandas/tests/series/methods/test_sort_values.py index 3b96c380ffd89..c8bbd92db16f7 100644 --- a/pandas/tests/series/methods/test_sort_values.py +++ b/pandas/tests/series/methods/test_sort_values.py @@ -216,14 +216,17 @@ def test_sort_values_validate_ascending(self): ser.sort_values(ascending="False") sorted_ser = ser.sort_values(ascending=False) - tm.assert_numpy_array_equal(sorted_ser.values, expected) + result = sorted_ser.values + tm.assert_numpy_array_equal(result, expected) sorted_ser = ser.sort_values(ascending=0) - tm.assert_numpy_array_equal(sorted_ser.values, expected) + result = sorted_ser.values + tm.assert_numpy_array_equal(result, expected) expected = expected[::-1] sorted_ser = ser.sort_values(ascending=1) - tm.assert_numpy_array_equal(sorted_ser.values, expected) + result = sorted_ser.values + tm.assert_numpy_array_equal(result, expected) class TestSeriesSortingKey: From 74ee732822274c238252bfd4a0af8e32c83c9a38 Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 25 Jul 2021 15:20:08 +0530 Subject: [PATCH 5/7] Changed object values as per the review --- pandas/tests/frame/methods/test_sort_values.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pandas/tests/frame/methods/test_sort_values.py b/pandas/tests/frame/methods/test_sort_values.py index 76d675ee21007..5aae51bab3ae7 100644 --- a/pandas/tests/frame/methods/test_sort_values.py +++ b/pandas/tests/frame/methods/test_sort_values.py @@ -879,14 +879,14 @@ def test_sort_values_validate_ascending(self): 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.loc[df.index[r_indexer]] + result = df.sort_values(by="D", ascending=False) + tm.assert_frame_equal(result, expected) - 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.loc[df.index[r_indexer]] + result = df.sort_values(by="D", ascending=0) + tm.assert_frame_equal(result, expected) - expected_df = df.loc[df.index[indexer]] - sorted_df = df.sort_values(by="D", ascending=1) - tm.assert_frame_equal(sorted_df, expected_df) + expected = df.loc[df.index[indexer]] + result = df.sort_values(by="D", ascending=1) + tm.assert_frame_equal(result, expected) From 9548857a72c378fcf9103886f48e7fbc59bd2f11 Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 25 Jul 2021 16:04:49 +0530 Subject: [PATCH 6/7] Restructured validate ascendingfor series --- .../tests/series/methods/test_sort_values.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/pandas/tests/series/methods/test_sort_values.py b/pandas/tests/series/methods/test_sort_values.py index c8bbd92db16f7..adc578d948163 100644 --- a/pandas/tests/series/methods/test_sort_values.py +++ b/pandas/tests/series/methods/test_sort_values.py @@ -206,25 +206,24 @@ 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): + def test_sort_values_validate_ascending_for_value_error(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) - result = sorted_ser.values - tm.assert_numpy_array_equal(result, expected) + @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=0) - result = sorted_ser.values - tm.assert_numpy_array_equal(result, expected) + sorted_ser = ser.sort_values(ascending=ascending) + if not ascending: + expected = expected[::-1] - expected = expected[::-1] - sorted_ser = ser.sort_values(ascending=1) result = sorted_ser.values tm.assert_numpy_array_equal(result, expected) From 9d394ece743caebe065ab02f4cf6a8a0328112b4 Mon Sep 17 00:00:00 2001 From: Rahul Date: Sun, 25 Jul 2021 16:13:42 +0530 Subject: [PATCH 7/7] Restructured validate ascending for frames --- pandas/tests/frame/methods/test_sort_values.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/pandas/tests/frame/methods/test_sort_values.py b/pandas/tests/frame/methods/test_sort_values.py index 5aae51bab3ae7..e104617552efc 100644 --- a/pandas/tests/frame/methods/test_sort_values.py +++ b/pandas/tests/frame/methods/test_sort_values.py @@ -869,24 +869,22 @@ def test_sort_values_pos_args_deprecation(self): expected = DataFrame({"a": [1, 2, 3]}) tm.assert_frame_equal(result, expected) - def test_sort_values_validate_ascending(self): + def test_sort_values_validate_ascending_for_value_error(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.loc[df.index[r_indexer]] - result = df.sort_values(by="D", ascending=False) - tm.assert_frame_equal(result, expected) + @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 - expected = df.loc[df.index[r_indexer]] - result = df.sort_values(by="D", ascending=0) - tm.assert_frame_equal(result, expected) + if not ascending: + indexer = indexer[::-1] expected = df.loc[df.index[indexer]] - result = df.sort_values(by="D", ascending=1) + result = df.sort_values(by="D", ascending=ascending) tm.assert_frame_equal(result, expected)