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

feat(rust!,python): Add Series.cat.uses_lexical_ordering #10325

Merged
merged 5 commits into from
Aug 7, 2023
Merged

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Aug 6, 2023

Closes #5671, supersedes #5705, blocker for #10267

I didn't want to rebase the existing PR, so here's a fresh PR.

WARNING: This functionality is considered experimental for now.

Changes:

  • On the Rust side:
    • Rename set_lexical_sorted to set_lexical_ordering.
    • Rename bit setting LEXICAL_SORT to LEXICAL_ORDERING.
    • Rename use_lexical_sort to uses_lexical_ordering and make it public outside the crate
  • On the Python side:
    • Expose the uses_lexical_ordering method for categorical Series.

I am really looking for a better name for this method, but I'm struggling to come up with something good. Help is appreciated!
I initially thought is_ordered would be appropriate. That would be the reverse of this - an ordered categorical sorts by physical type rather than lexical. -> Chose a sensible name for now and marked as experimental.

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Aug 6, 2023
@stinodego stinodego marked this pull request as draft August 7, 2023 05:46
@orlp
Copy link
Collaborator

orlp commented Aug 7, 2023

Why not just get_ordering returning "lexical" or "physical" just like the input of set_ordering?

@stinodego
Copy link
Member Author

stinodego commented Aug 7, 2023

Why not just get_ordering returning "lexical" or "physical" just like the input of set_ordering?

That makes sense with the current API, though I don't like having to check the result versus a string, when I know there are only two options. It's both inefficient and easy to mess up.

We should probably change set_ordering too, at some point. For the same reason.

I think I'll just slap an "experimental" tag on this for now and merge it as-is. We can revisit later.

@stinodego stinodego marked this pull request as ready for review August 7, 2023 09:51
@stinodego stinodego marked this pull request as draft August 7, 2023 10:22
@stinodego
Copy link
Member Author

@ritchie46 I chose a slightly different name / renamed related functionality on the Rust side. Are you OK with this?

@stinodego stinodego marked this pull request as ready for review August 7, 2023 10:34
@stinodego stinodego changed the title feat(rust,python): Add Series.cat.uses_lexical_sort feat(rust,python): Add Series.cat.uses_lexical_ordering Aug 7, 2023
@stinodego stinodego changed the title feat(rust,python): Add Series.cat.uses_lexical_ordering feat(rust!,python): Add Series.cat.uses_lexical_ordering Aug 7, 2023
@ritchie46
Copy link
Member

@ritchie46 I chose a slightly different name / renamed related functionality on the Rust side. Are you OK with this?

Yeap.

@github-actions github-actions bot added the breaking rust Change that breaks backwards compatibility for the Rust crate label Aug 7, 2023
@stinodego stinodego merged commit aea8015 into main Aug 7, 2023
27 checks passed
@stinodego stinodego deleted the cat-ordered2 branch August 7, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking rust Change that breaks backwards compatibility for the Rust crate enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Series.cat.categories and Series.cat.ordered
3 participants