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

Add segmented_gather(list_column, gather_list) #7003

Merged
merged 28 commits into from
Jan 12, 2021
Merged

Add segmented_gather(list_column, gather_list) #7003

merged 28 commits into from
Jan 12, 2021

Conversation

karthikeyann
Copy link
Contributor

@karthikeyann karthikeyann commented Dec 14, 2020 •
edited
Loading

closes #6542

  • Add segmented_gather(list, list)
  • Add unit tests
  • Documentation

@karthikeyann karthikeyann added 2 - In Progress Currently a work in progress feature request New feature or request non-breaking Non-breaking change labels Dec 14, 2020
@codecov
Copy link

codecov bot commented Dec 14, 2020 •
edited
Loading

Codecov Report

Merging #7003 (4d7d594) into branch-0.18 (2a2b4d6) will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #7003      +/-   ##
===============================================
+ Coverage        82.01%   82.10%   +0.08%     
===============================================
  Files               96       97       +1     
  Lines            16340    16493     +153     
===============================================
+ Hits             13402    13541     +139     
- Misses            2938     2952      +14
Impacted Files Coverage Δ
python/cudf/cudf/utils/gpu_utils.py 53.65% <0.00%> (-4.88%) ⬇️
python/cudf/cudf/core/abc.py 87.23% <0.00%> (-4.26%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.17% <0.00%> (-0.39%) ⬇️
python/cudf/cudf/core/frame.py 89.97% <0.00%> (-0.38%) ⬇️
python/cudf/cudf/core/column/timedelta.py 89.16% <0.00%> (-0.38%) ⬇️
python/cudf/cudf/core/column/lists.py 91.66% <0.00%> (-0.09%) ⬇️
python/cudf/cudf/core/reshape.py 91.00% <0.00%> (-0.04%) ⬇️
python/cudf/cudf/core/series.py 91.08% <0.00%> (-0.02%) ⬇️
python/cudf/cudf/__init__.py 100.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 95.23% <0.00%> (ø)
... and 18 more

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 2a2b4d6...4d7d594. Read the comment docs.

@karthikeyann karthikeyann marked this pull request as ready for review December 17, 2020 11:41
@karthikeyann karthikeyann requested review from a team as code owners December 17, 2020 11:41
@karthikeyann @vuule
review comments (vuule)
975e144 
Co-authored-by: Vukasin Milovanovic <vukasin.milovanovic.87@gmail.com>
nvdbaranec
nvdbaranec requested changes Dec 18, 2020
View reviewed changes
cpp/src/lists/copying/segmented_gather.cu Outdated Show resolved Hide resolved
cpp/src/lists/copying/segmented_gather.cu Outdated
auto child_gather_index_begin = child_gather_index->mutable_view().begin<size_type>();
auto child_gather_index_end = child_gather_index->mutable_view().end<size_type>();

thrust::upper_bound(rmm::exec_policy(stream),
Copy link
Contributor

Choose a reason for hiding this comment

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

This wouldn't work if the gather_map was sliced (which is probably unlikely, but you never know). Similar to below, you need to start at the index indicated by
gather_map.offsets().begin<size_type>() + gather_map.offset()
and subtract
(gather_map.offsets().begin<size_type>() + gather_map.offset())[0]
from each value

Copy link
Contributor

Choose a reason for hiding this comment

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

Or alternately, throw an exception if someone passes a gather_map with a positive .offset()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added sliced gather_map support.

cpp/src/lists/copying/segmented_gather.cu Show resolved Hide resolved
cpp/tests/copying/segmented_gather_list_tests.cpp Show resolved Hide resolved
@karthikeyann
Copy link
Contributor Author

rerun tests

@karthikeyann karthikeyann added the 4 - Needs Review Waiting for reviewer to review or respond label Dec 29, 2020
@karthikeyann karthikeyann requested a review from a team as a code owner January 5, 2021 05:29
@karthikeyann karthikeyann added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Jan 11, 2021
@rapids-bot rapids-bot bot merged commit 87e414c into rapidsai:branch-0.18 Jan 12, 2021
@shwina shwina mentioned this pull request Feb 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@vuule vuule vuule approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Segmented gather
7 participants
@karthikeyann @mike-wendt @ajschmidt8 @jrhemstad @vuule @nvdbaranec @harrism