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
18 changes: 4 additions & 14 deletions pandas/core/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -11776,20 +11776,10 @@ def _doc_params(cls):
"""


def validate_ascending(
ascending: Union[bool, Sequence[bool]],
) -> Union[bool, Sequence[bool]]:
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)):
_check_ascending_element(ascending)
return ascending
return validate_bool_kwarg(ascending, "ascending", **kwargs)

for item in ascending:
_check_ascending_element(item)
return ascending


def _check_ascending_element(value):
"""Ensure that each item in ``ascending`` kwarg is either bool or int."""
if value is None or not isinstance(value, (bool, int)):
raise ValueError("ascending must be either a bool or a sequence of bools")
return [validate_bool_kwarg(item, "ascending", **kwargs) for item in ascending]
2 changes: 1 addition & 1 deletion pandas/tests/frame/methods/test_sort_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ 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 = "ascending must be either a bool or a sequence of bools"
match = 'For argument "ascending" expected type bool'
with pytest.raises(ValueError, match=match):
df.sort_index(axis=0, ascending=ascending, na_position="first")

Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/series/methods/test_sort_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ def test_sort_index_ascending_list(self):
)
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 = "ascending must be either a bool or a sequence of bools"
match = 'For argument "ascending" expected type bool'
with pytest.raises(ValueError, match=match):
ser.sort_index(ascending=ascending)

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