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

Expose stable versions of libcudf sort routines #13634

Merged
merged 5 commits into from
Jul 4, 2023
Merged

Expose stable versions of libcudf sort routines #13634

merged 5 commits into from
Jul 4, 2023

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Jun 28, 2023

Description

As preparation for addressing parts of #13557, this just exposes the existing sort-by and stable-sort-by routines from libcudf.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence- wence- added Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 28, 2023
@wence- wence- requested a review from a team as a code owner June 28, 2023 15:51
bdice
bdice requested changes Jun 28, 2023
View reviewed changes
python/cudf/cudf/_lib/sort.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/sort.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/sort.pyx Show resolved Hide resolved
@wence-
Make stable a required keyword argument
cade288 
This also points out places where previously we had a default unstable
sort. So switch the default to be whether we are in
pandas-compatibility mode.
@wence-
Fix docstrings
b1e90dc
@wence-
Copy link
Contributor Author

wence- commented Jun 29, 2023

As part of the stable keyword argument case, I realise that there is zero consistency in the handling of the column_order/null_precedence arguments. Some functions allow defaults, others don't. And the defaults in the cython wrapper match those of libcudf, but the null precedence argument defaults are opposite to the defaults in the higher-level cudf pandas wrappers.

In the interest of keeping this small, I'll open an issue.

vyasr
vyasr approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Looks good aside from some minor doc improvements so approving with a few suggestions.

python/cudf/cudf/_lib/sort.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/sort.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/sort.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/sort.pyx Outdated Show resolved Hide resolved
@wence-
Copy link
Contributor Author

wence- commented Jun 30, 2023

Thanks for the proof-reading @vyasr. Any further issues @bdice?

bdice
bdice previously requested changes Jun 30, 2023
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Proposing always using stable sort. Otherwise LGTM!

python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
@wence- wence- dismissed bdice’s stale review July 3, 2023 10:00

Addressed comment.

@wence-
Copy link
Contributor Author

wence- commented Jul 4, 2023

/merge

@rapids-bot rapids-bot bot merged commit 55b9bfc into rapidsai:branch-23.08 Jul 4, 2023
53 checks passed
@wence- wence- deleted the wence/fea/sort-by-key branch July 4, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice left review comments

@vyasr vyasr vyasr approved these changes

@shwina shwina Awaiting requested review from shwina shwina is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@brandon-b-miller brandon-b-miller Awaiting requested review from brandon-b-miller brandon-b-miller is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees
No one assigned
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
cuDF/Dask/Numba/UCX
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wence- @vyasr @bdice