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

[FEA] Remove inconsistencies in cython wrappers when handling order/null-precedence #14492

Open
wence- opened this issue Nov 24, 2023 · 4 comments
Labels
feature request New feature or request good first issue Good for newcomers Python Affects Python cuDF API.

Comments

@wence-
Copy link
Contributor

wence- commented Nov 24, 2023

Where libcudf search/sort functions accept a table_view as input, one must specify the order (ascending or descending) and null precedence (beginning or end) as a std::vector of length equal to the number of columns in the table_view (or else an empty such vector to used libcudf's defaults).

In the cudf cython wrapping of these functions we are, in contrast, inconsistent in the way we handle these arguments. Some functions accept a list for both order and null precedence (e.g. libcudf.sort.sort); some accept a list for order but only a single value for null precedence (e.g. libcudf.sort.order_by); some accept a list for neither order nor null precedence (e.g. libcudf.search.search_sorted).

This should be cleaned up and all such functions should uniformly accept a list for both order and null precedence. Higher-level python functions that operate on single columns and call the table interfaces should be responsible for any argument munging.

@wence- wence- added feature request New feature or request Python Affects Python cuDF API. tech debt good first issue Good for newcomers labels Nov 24, 2023
@wence- wence- changed the title [FEA] Remove inconsistencies in cython wrappers when handling of order/null-precedence [FEA] Remove inconsistencies in cython wrappers when handling order/null-precedence Nov 27, 2023
@MananDoshi1301
Copy link

If this issue is still open and relevant, I would like to give it a try @wence-
Thank you!

@shwina
Copy link
Contributor

shwina commented Feb 13, 2024

Hi @MananDoshi1301 - we are in the process of refactoring our Cython internals. This effort is described here:

#13921

So while you're touching sort.pyx, I wonder if it's worth also just refactoring it out into the new pylibcudf package. Is that something you would be interested in taking on?

Happy to answer any questions you might have here, or on the RAPIDS Slack!

@MananDoshi1301
Copy link

Yes absolutely, I'd love to do it. Thanks for the heads up @shwina!

@shwina
Copy link
Contributor

shwina commented Feb 15, 2024

Apologies - it looks like sort was ported over to pylibcudf as part of #15011.

If you're interested in contributing, many of the open Python issues should be quite approachable:

https://github.com/rapidsai/cudf/issues?q=is%3Aopen+is%3Aissue+label%3A%22cuDF+%28Python%29%22

If anything piques your interest, please let us know!

@vyasr vyasr removed the tech debt label Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request good first issue Good for newcomers Python Affects Python cuDF API.
Projects
Status: In Progress
Development

No branches or pull requests

4 participants