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

Enable passing a sequence for the index argument to .list.get() #10564

Merged
merged 22 commits into from
Apr 12, 2022
Merged

Enable passing a sequence for the index argument to .list.get() #10564

merged 22 commits into from
Apr 12, 2022

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Mar 31, 2022 •
edited
Loading

Closes #10552.

Depends on #10547

@shwina shwina requested review from a team as code owners March 31, 2022 21:55
@github-actions github-actions bot added the Python Affects Python cuDF API. label Mar 31, 2022
@shwina shwina added non-breaking Non-breaking change feature request New feature or request labels Mar 31, 2022
@codecov
Copy link

codecov bot commented Mar 31, 2022 •
edited
Loading

Codecov Report

Merging #10564 (1e5a2a8) into branch-22.06 (bf4ffc9) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           branch-22.06   #10564      +/-   ##
================================================
+ Coverage         86.33%   86.37%   +0.04%     
================================================
  Files               140      140              
  Lines             22289    22290       +1     
================================================
+ Hits              19244    19254      +10     
+ Misses             3045     3036       -9
Impacted Files Coverage Δ
python/cudf/cudf/core/column/lists.py 92.79% <100.00%> (+1.38%) ⬆️
python/dask_cudf/dask_cudf/backends.py 86.44% <100.00%> (ø)
python/dask_cudf/dask_cudf/sorting.py 93.03% <100.00%> (ø)
python/dask_cudf/dask_cudf/tests/test_accessor.py 99.20% <100.00%> (+0.78%) ⬆️
python/cudf/cudf/core/column/string.py 89.10% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.72% <0.00%> (+0.22%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/dask_cudf/dask_cudf/accessors.py 96.15% <0.00%> (+3.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 012af64...1e5a2a8. Read the comment docs.

@shwina shwina mentioned this pull request Apr 1, 2022
Merged
@bdice
Copy link
Contributor

bdice commented Apr 3, 2022

@shwina I'm going to wait until we merge #10547 to review this PR.

bdice
bdice requested changes Apr 6, 2022
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.

I have a few comments below. Please rebase this now that the dependent PR has been merged.

python/cudf/cudf/core/column/lists.py Show resolved Hide resolved
python/cudf/cudf/core/column/lists.py Outdated Show resolved Hide resolved
python/cudf/cudf/tests/test_list.py Show resolved Hide resolved
@shwina shwina mentioned this pull request Apr 6, 2022
Open
bdice
bdice approved these changes Apr 7, 2022
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.

I like the changes you made. I noted one duplicate test, otherwise LGTM! :)

python/cudf/cudf/tests/test_list.py Outdated Show resolved Hide resolved
vyasr
vyasr approved these changes Apr 8, 2022
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.

LGTM. There are still a few unresolved conversations, nothing blocking but probably worth double-checking that everything there has been addressed now.

python/cudf/cudf/_lib/lists.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/core/column/lists.py Show resolved Hide resolved
@bdice bdice added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Apr 12, 2022
@shwina
Copy link
Contributor Author

shwina commented Apr 12, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 2348277 into rapidsai:branch-22.06 Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@charlesbluca charlesbluca charlesbluca approved these changes

@vyasr vyasr vyasr approved these changes

@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
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Extend .list.get() method to accept a sequence of integer indices
4 participants
@shwina @bdice @vyasr @charlesbluca