-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
ERR: better error message with invalid argument in sort_values #42358
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
Conversation
Hello @deej197! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-07-03 19:22:47 UTC |
pandas/core/series.py
Outdated
@@ -3441,8 +3441,10 @@ def sort_values( | |||
) | |||
ascending = ascending[0] | |||
|
|||
if not is_bool(ascending): | |||
raise ValueError("ascending must be boolean") | |||
#if not is_bool(ascending): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_bool(ascending) is equivalent to isinstance(ascending, (bool, np.bool_)
, so this shouldn't make a difference. if im wrong and this does make a difference, this needs a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see this issue open #41634. if this is not resolved, then is_bool function is not working as expected.
regarding the comment, wasnt aware of the same as this is my first contribution. will note this for future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deej197 Isn't this issue for DataFrame not series?
Line 6208 in dad3e7f
def sort_values( # type: ignore[override] |
You might want to look into using validate_ascending
pandas/pandas/util/_validators.py
Line 421 in dad3e7f
def validate_ascending( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lithomas1 I can make the changes in this validate_ascending function , but i am stuck in running tests locally. I tried replacing the pandas package in C:\Users\usename\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.7_qbz5n2kfra8p0\LocalCache\local-packages\Python37\site-packages with the modified package, but i dont see the changes being applied.
appreciate any suggestions on executing tests internally.
pandas/core/series.py
Outdated
#if not is_bool(ascending): | ||
# raise ValueError("ascending must be boolean") | ||
if type(ascending)!=bool: | ||
raise ValueError("ascending should of type bool") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there some words left out of this message? what's wrong with the existing one?
Thanks for the contribution @deej197, but unfortunately you won't be able to contribute with a proper development environment. If you could run this test locally you could see that:
And it's also better to start by opening an issue, finding a reproducible example, and when there is agreement with the solution, then open the PR (which would require tests and a release note, besides the code changes). I opened an issue myself, so you can see how would be better to start: #42434. Feel free to work on the issue, but as I said, you need a working development environment. Otherwise you won't be able to test what you're doing, and you'll be submitting obviously wrong PRs like this, that don't add value, and waste reviewers time. If you follow the contribution guide, you shouldn't have problems setting up the develoment environment, if something is not clear enough, feel free to open an issue to ask for clarification in the docs. |
added a if branch to check the ascending type, as the existing one accepts value with string as well instead of boolean.
k=df.sort_values(by=['col1'],ascending="False") does not give exception with str.
added piece of code to raise exception if the type is not boolean