Skip to content
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

Rename keyword argument reverse to ascending for sorting methods #5429

Closed
zundertj opened this issue Nov 5, 2022 · 2 comments · Fixed by #6914
Closed

Rename keyword argument reverse to ascending for sorting methods #5429

zundertj opened this issue Nov 5, 2022 · 2 comments · Fixed by #6914
Labels
breaking Change that breaks backwards compatibility enhancement New feature or an improvement of an existing feature python Related to Python Polars

Comments

@zundertj
Copy link
Collaborator

zundertj commented Nov 5, 2022

Problem description

Summary

In Polars, the sorting methods (on expressions/series/dataframes) have a keyword argument named reverse. I propose to change this name to ascending.

Rationale

The issue I have with reverse is that it does not tell me what "non reverse" / "normal" is. The docstring on for example DataFrame.sort also acknowledges this, because it says:

    reverse

        Reverse/descending sort.

I.e., reverse=descending, so non-reverse=ascending. You wouldn't know from the parameter name itself. Docs are good, but proper argument naming is better.

Pandas does use ascending. Pandas consistency is not a goal, but I feel Pandas has the better naming here.

PyArrow opts for the name order, and asks for a string to be passed in (ascending or descending). See for example array_sort_indices. As long as we can imagine having only two orderings, a bool is easier than a string imo, so I like the Pandas approach more.

Moreover, I don't see a drawback in adopting this terminology, other than that we have to do a one-time breaking release.

@zundertj zundertj added python Related to Python Polars breaking Change that breaks backwards compatibility enhancement New feature or an improvement of an existing feature labels Nov 5, 2022
@zundertj zundertj changed the title Rename keyword argument reverse to ascending Rename keyword argument reverse to ascending for sorting methods Nov 5, 2022
@zundertj
Copy link
Collaborator Author

zundertj commented Nov 6, 2022

(not knowing the Rust code that well) We could set the default for ascending to True, or if we go with descending to False, both will achieve ascending by default. Reason I picked ascending and not descending is that is what Pandas does. Probably a matter of taste.

@ritchie46
Copy link
Member

Yes, I have been thinking about this one as well. I think we should name it to descending so that the default in rust remains in ascending order.

If we do this we need to do both rust and python at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Change that breaks backwards compatibility enhancement New feature or an improvement of an existing feature python Related to Python Polars
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants