Skip to content

BUG: raise when sort_index with ascending=None #39451

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 14 commits into from
Feb 26, 2021
11 changes: 11 additions & 0 deletions pandas/core/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -4533,6 +4533,8 @@ def sort_index(

inplace = validate_bool_kwarg(inplace, "inplace")
axis = self._get_axis_number(axis)
ascending = validate_ascending(ascending)

target = self._get_axis(axis)

indexer = get_indexer_indexer(
Expand Down Expand Up @@ -11772,3 +11774,12 @@ def _doc_params(cls):
The required number of valid values to perform the operation. If fewer than
``min_count`` non-NA values are present the result will be NA.
"""


def validate_ascending(ascending):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move to pandas/util/validators.py

shouldn't this raise for a list-like case if something is wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. this shouldn't return anything

pls add some typing

Copy link
Member Author

Choose a reason for hiding this comment

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

It raises for a list-like case if any of the items in ascending list is not treated as a valid boolean.
This construct does just that.

return [validate_bool_kwarg(item, "ascending", **kwargs) for item in ascending]

Copy link
Contributor

Choose a reason for hiding this comment

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

this can simply be part of validate_bool_kwarg or another function but should be co-located with validate_bool_kwarg

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the function to pandas/util/_validators.py.
It is not clear for me at the moment how to make this validation anyhow different (as a part of validate_bool_kwarg) due to the fact that multiple types are acceptable for ascending.

"""Validate ``ascending`` kwargs for ``sort_index`` method."""
kwargs = {"none_allowed": False, "int_allowed": True}
if not isinstance(ascending, (list, tuple)):
return validate_bool_kwarg(ascending, "ascending", **kwargs)

return [validate_bool_kwarg(item, "ascending", **kwargs) for item in ascending]
16 changes: 16 additions & 0 deletions pandas/tests/frame/methods/test_sort_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,22 @@ def test_sort_index_with_categories(self, categories):
)
tm.assert_frame_equal(result, expected)

@pytest.mark.parametrize(
"ascending",
[
None,
(True, None),
(False, "True"),
Copy link
Member

Choose a reason for hiding this comment

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

can you make these lists

Copy link
Member

Choose a reason for hiding this comment

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

and maybe add an empty list too

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently an empty list does not raise an error, so the test fails.
Should I change the code, so that an empty list raises?

],
)
def test_sort_index_ascending_bad_value_raises(self, ascending):
df = DataFrame(np.arange(64))
length = len(df.index)
df.index = [(i - length / 2) % length for i in range(length)]
match = 'For argument "ascending" expected type bool'
with pytest.raises(ValueError, match=match):
df.sort_index(axis=0, ascending=ascending, na_position="first")


class TestDataFrameSortIndexKey:
def test_sort_multi_index_key(self):
Expand Down
14 changes: 14 additions & 0 deletions pandas/tests/series/methods/test_sort_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,20 @@ def test_sort_index_ascending_list(self):
expected = ser.iloc[[0, 4, 1, 5, 2, 6, 3, 7]]
tm.assert_series_equal(result, expected)

@pytest.mark.parametrize(
"ascending",
[
None,
(True, None),
(False, "True"),
],
)
def test_sort_index_ascending_bad_value_raises(self, ascending):
ser = Series(range(10), index=[0, 3, 2, 1, 4, 5, 7, 6, 8, 9])
match = 'For argument "ascending" expected type bool'
with pytest.raises(ValueError, match=match):
ser.sort_index(ascending=ascending)


class TestSeriesSortIndexKey:
def test_sort_index_multiindex_key(self):
Expand Down
36 changes: 33 additions & 3 deletions pandas/util/_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,39 @@ def validate_args_and_kwargs(fname, args, kwargs, max_fname_arg_count, compat_ar
validate_kwargs(fname, kwargs, compat_args)


def validate_bool_kwarg(value, arg_name):
""" Ensures that argument passed in arg_name is of type bool. """
if not (is_bool(value) or value is None):
def validate_bool_kwarg(value, arg_name, none_allowed=True, int_allowed=False):
"""
Ensure that argument passed in arg_name can be interpreted as boolean.

Parameters
----------
value : bool
Value to be validated.
arg_name : str
Name of the argument. To be reflected in the error message.
none_allowed : bool, optional
Copy link
Member

Choose a reason for hiding this comment

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

add default

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

Whether to consider None to be a valid boolean.
int_allowed : bool, optional
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

Whether to consider integer value to be a valid boolean.

Returns
-------
value
The same value as input.

Raises
------
ValueError
If the value is not a valid boolean.
"""
good_value = is_bool(value)
if none_allowed:
good_value = good_value or value is None

if int_allowed:
good_value = good_value or isinstance(value, int)

if not good_value:
raise ValueError(
f'For argument "{arg_name}" expected type bool, received '
f"type {type(value).__name__}."
Expand Down