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.

Sign up for GitHub

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

Jump to bottom

refactor(rust, python): Rename kwarg reverse to descending #6914

Merged
merged 1 commit into from Feb 20, 2023
Merged

refactor(rust, python): Rename kwarg reverse to descending #6914

merged 1 commit into from Feb 20, 2023

Conversation

zundertj
Copy link
Collaborator

@zundertj zundertj commented Feb 15, 2023 •
edited

This will trigger a lot of deprecation warnings in Python, and possibly errors on the Rust side.

Closes #5429.

@zundertj zundertj marked this pull request as draft February 15, 2023 21:50
@zundertj zundertj changed the title Rename kwarg reverse to descending refactor(rust,python): Rename kwarg reverse to descending Feb 16, 2023
@github-actions github-actions bot added python Related to Python Polars refactor Code improvements that do not change functionality rust Related to Rust Polars labels Feb 16, 2023
@zundertj zundertj changed the title refactor(rust,python): Rename kwarg reverse to descending refactor(rust, python): Rename kwarg reverse to descending Feb 16, 2023
@zundertj zundertj force-pushed the reverse_to_descending branch 4 times, most recently from 605f1c5 to 272d5bc Compare February 18, 2023 10:01
@zundertj
Copy link
Collaborator Author

Ok, I am nearly there, but getting stuck with (python) Series.rank and Series.top_k; these now return None. The commonality is that neither of those has a function body in Python, so it drops directly into the Rust layer. I think I made the right adjustments there, but clearly not enough. Could someone with more Rust experience have a look?

@stinodego
Copy link
Member

stinodego commented Feb 19, 2023 •
edited

Adding the @deprecated_alias decorator unfortunately breaks the dispatch of Series -> Expr. So you'll have to add the dispatch to the method manually. See my other comment.

@zundertj zundertj marked this pull request as ready for review February 19, 2023 09:16
@zundertj
Copy link
Collaborator Author

All good now. This PR is probably merge conflict prone, but happy to fix once we decide to merge all the breaking stuff in.

stinodego
stinodego approved these changes Feb 19, 2023
View reviewed changes
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Looks really clean 👍 I think this can be merged!

@ritchie46
Copy link
Member

Can you do a rebase? I have touched reverse #7021

@ritchie46
Copy link
Member

And a great improvement!

@zundertj
Rename kwarg reverse to descending
47e7afd 
Amend previous commit

Update

Revert

Fix Rust lint errors

Fix bugs

Use deprecated_alias

Fix more
@stinodego
Copy link
Member

I'm going to merge this before it gathers any more conflicts. Thanks @zundertj !

@stinodego stinodego merged commit c7b8599 into pola-rs:master Feb 20, 2023
josemasar pushed a commit to josemasar/polars that referenced this pull request Feb 21, 2023
@zundertj @josemasar
refactor(rust, python): Rename kwarg reverse to descending (pola-rs#6914
0860b98 
)
zundertj added a commit to zundertj/polars that referenced this pull request Feb 25, 2023
@zundertj
fix(python): Fix Series.argsort
ad50119 
Resolved pola-rs#7182.

I think this is due to the decorator on reverse => descending kwarg introduced in pola-rs#6914.
@zundertj zundertj mentioned this pull request Feb 25, 2023
Merged
This was referenced Apr 28, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stinodego stinodego stinodego approved these changes

Assignees
No one assigned
Labels
python Related to Python Polars refactor Code improvements that do not change functionality rust Related to Rust Polars
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename keyword argument reverse to ascending for sorting methods
3 participants
@zundertj @stinodego @ritchie46