-
Notifications
You must be signed in to change notification settings - Fork 915
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
Add lists.sort_values
API
#7657
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-0.19 #7657 +/- ##
===============================================
+ Coverage 81.86% 82.48% +0.61%
===============================================
Files 101 101
Lines 16884 17426 +542
===============================================
+ Hits 13822 14373 +551
+ Misses 3062 3053 -9
Continue to review full report at Codecov.
|
python/cudf/cudf/_lib/lists.pyx
Outdated
@@ -58,3 +63,22 @@ def explode_outer(Table tbl, int explode_column_idx, bool ignore_index=False): | |||
column_names=tbl._column_names, | |||
index_names=None if ignore_index else tbl._index_names | |||
) | |||
|
|||
|
|||
def sort_lists(Column col, object order_enum, object null_order_enum): |
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 there a way to more tightly type the enums here? I thought we had a pattern fo handling enum values like this elsewhere but I could be mistaken.
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.
Unfortunately no. I think typically we just pass a Python object to Cython, where we then plumb it through to the appropriate C++ type. See for example https://github.com/rapidsai/cudf/blob/branch-0.19/python/cudf/cudf/_lib/sort.pyx#L26.
I'd suggest we do something similar here, where the Cython API shouldn't expect a Python enum, but rather something like a string or a bool. That encapsulates things a little better, making the use of an enum an implementation detail.
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.
Apologies, @isVoid I got this backwards and you had it right from the beginning
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 think this needs to be addressed before merging
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.
Got it. Sorry misunderstood on this one!
@gpucibot merge |
Closes #7467
Introduces list method
list.sort_values
. Sorts each list of a LIST column based on given criterion. This method signature is aligned withSeries.sort_values
. Example:This PR also includes exposing
ListMethods
to docs and a small docstring fix tocudf.Series
.