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

Migrate lists/combine to pylibcudf #15928

Merged
merged 8 commits into from
Jun 12, 2024
Merged

Migrate lists/combine to pylibcudf #15928

merged 8 commits into from
Jun 12, 2024

Conversation

Matt711
Copy link
Contributor

@Matt711 Matt711 commented Jun 5, 2024 •
edited
Loading

Description

Part of #15162. concatenate_rows, concatenate_list_elements

Checklist

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

@Matt711 Matt711 self-assigned this Jun 5, 2024
@github-actions github-actions bot added Python Affects Python cuDF API. pylibcudf Issues specific to the pylibcudf package labels Jun 5, 2024
@Matt711 Matt711 added non-breaking Non-breaking change feature request New feature or request and removed Python Affects Python cuDF API. labels Jun 5, 2024
@github-actions github-actions bot added the Python Affects Python cuDF API. label Jun 5, 2024
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 5, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Matt711
Copy link
Contributor Author

Matt711 commented Jun 5, 2024 •
edited
Loading

Note to me: I thought I was getting the wrong result from pylibcudf.concatenate_list_elements because when I passed dropna=False, I thought [[[1, 2], [3, 4], [5]], [[6], [None], [7, 8, 9]]] would return [[1, 2, 3, 4, 5], None] but I see in the docs the list element needs to be null. So, [[[1, 2], [3, 4], [5]], [[6], None, [7, 8, 9]]] would return [[1, 2, 3, 4, 5], None]

@Matt711 Matt711 marked this pull request as ready for review June 5, 2024 22:31
@Matt711 Matt711 requested a review from a team as a code owner June 5, 2024 22:31
@Matt711 Matt711 requested review from mroeschke and lithomas1 June 5, 2024 22:31
lithomas1
lithomas1 reviewed Jun 5, 2024
View reviewed changes
Copy link
Contributor

@lithomas1 lithomas1 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good generally!

I left some initial comments.

python/cudf/cudf/_lib/pylibcudf/lists.pyx Show resolved Hide resolved
python/cudf/cudf/_lib/pylibcudf/lists.pxd Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Jun 11, 2024

/ok to test

vyasr
vyasr approved these changes Jun 11, 2024
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.

Some minor doc suggestions, otherwise looks good. Feel free to merge once you've applied or rejected my suggestions as you see fit.

python/cudf/cudf/_lib/pylibcudf/lists.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/pylibcudf/lists.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/pylibcudf/lists.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/pylibcudf_tests/test_lists.py Show resolved Hide resolved
@Matt711 Matt711 removed the request for review from mroeschke June 11, 2024 21:28
lithomas1
lithomas1 approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@lithomas1 lithomas1 left a comment

Choose a reason for hiding this comment

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

This LGTM as well.
Just recommend two small changes in the asserts for the tests.

thanks @Matt711!

python/cudf/cudf/pylibcudf_tests/test_lists.py Outdated Show resolved Hide resolved
python/cudf/cudf/pylibcudf_tests/test_lists.py Outdated Show resolved Hide resolved
@Matt711 Matt711 force-pushed the pylibcudf-lists-combine branch from f8f8113 to f6d12e4 Compare June 11, 2024 21:51
@Matt711
Copy link
Contributor Author

Matt711 commented Jun 11, 2024

/ok to test

@Matt711
Copy link
Contributor Author

Matt711 commented Jun 12, 2024

/merge

@rapids-bot rapids-bot bot merged commit d2cd1d4 into rapidsai:branch-24.08 Jun 12, 2024
75 checks passed
@Matt711 Matt711 mentioned this pull request Jul 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lithomas1 lithomas1 lithomas1 approved these changes

@vyasr vyasr vyasr approved these changes

Assignees

@Matt711 Matt711

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
cuDF Python
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Matt711 @vyasr @lithomas1