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

Rewrite cudf internals using pylibcudf groupby #14946

Merged
merged 13 commits into from
Feb 5, 2024

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 1, 2024

Description

This PR builds on #14945 to use pylibcudf's groupby in cudf's internals. It should not be merged until after that PR.

Checklist

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

@vyasr vyasr added feature request New feature or request non-breaking Non-breaking change labels Feb 1, 2024
@vyasr vyasr self-assigned this Feb 1, 2024
@github-actions github-actions bot added Python Affects Python cuDF API. CMake CMake build issue labels Feb 1, 2024
@vyasr vyasr marked this pull request as ready for review February 2, 2024 02:01
@vyasr vyasr requested a review from a team as a code owner February 2, 2024 02:01
@github-actions github-actions bot removed the CMake CMake build issue label Feb 2, 2024
Comment on lines +32 to +36
This class is functionally polymorphic and can represent either an
aggregation or a scan depending on the algorithm it is used with. For
details on the libcudf types it converts to, see
:cpp:class:`cudf::groupby::aggregation_request` and
:cpp:class:`cudf::groupby::scan_request`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this maybe leaking too much implementation detail for a docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah it may be. In the interest of speed I'm going to merge this PR now, but I do think all of pylibcudf's documentation deserves a thorough review before we actually expose the API. I'll make a note that we should check on this docstring in particular when doing that.

Copy link
Contributor

@shwina shwina left a comment

Choose a reason for hiding this comment

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

Mostly looks great! The Cython layer is so much cleaner now.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 5, 2024

/merge

@rapids-bot rapids-bot bot merged commit de1da2b into rapidsai:branch-24.04 Feb 5, 2024
73 checks passed
@vyasr vyasr deleted the feat/use_pylibcudf_groupby branch February 5, 2024 18:48
@vyasr vyasr added the pylibcudf Issues specific to the pylibcudf package label May 28, 2024
This pull request was closed.
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 non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants