Skip to content

Support scattering list_scalar - #8256

Merged
rapids-bot[bot] merged 18 commits into
NVIDIA:branch-21.06from
isVoid:scatter_list_scalars
May 24, 2021
Merged

Support scattering list_scalar#8256
rapids-bot[bot] merged 18 commits into
NVIDIA:branch-21.06from
isVoid:scatter_list_scalars

Conversation

@isVoid

@isVoid isVoid commented May 15, 2021

Copy link
Copy Markdown
Contributor

This PR adds support for scattering list_scalar into a LIST column.

This PR refactors current scatter for LIST column interface and allows column and scalar scatter share the same code path.

This PR also removes all debugging codes in scatter.cuh

@isVoid isVoid self-assigned this May 15, 2021
@github-actions github-actions Bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels May 15, 2021
@isVoid isVoid added 2 - In Progress Currently a work in progress feature request New feature or request CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change and removed CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels May 15, 2021
@harrism harrism changed the title Supports scattering list_scalar Support scattering list_scalar May 17, 2021
@isVoid
isVoid marked this pull request as ready for review May 18, 2021 06:48
@isVoid
isVoid requested a review from a team as a code owner May 18, 2021 06:48
@isVoid
isVoid requested review from davidwendt and ttnghia May 18, 2021 06:48
@isVoid isVoid added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels May 18, 2021

@davidwendt davidwendt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove the debug code. I believe scatter is used as a primitive in other cudf algorithms. This code will be difficult to maintain and frustrating to ignore when not specifically debugging this scatter. It also increases the size of the debug version of libcudf.so which is already getting to be too large for the debugger.

Comment thread cpp/tests/copying/scatter_list_scalar_tests.cpp Outdated
isVoid and others added 2 commits May 19, 2021 16:02
Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>
@isVoid

isVoid commented May 20, 2021

Copy link
Copy Markdown
Contributor Author

rerun tests

@isVoid
isVoid requested a review from davidwendt May 20, 2021 15:57
Comment thread cpp/tests/copying/scatter_list_scalar_tests.cpp Outdated
Comment thread cpp/tests/copying/scatter_list_scalar_tests.cpp Outdated
@isVoid isVoid added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels May 20, 2021
@isVoid

isVoid commented May 21, 2021

Copy link
Copy Markdown
Contributor Author

Looks like there are some failed tests, investigating.

@isVoid isVoid added 0 - Waiting on Author Waiting for author to respond to review and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels May 21, 2021
@isVoid

isVoid commented May 21, 2021

Copy link
Copy Markdown
Contributor Author

Welp, I broke the column scatter interface during the refactor. Should be fixed.

@isVoid isVoid added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 0 - Waiting on Author Waiting for author to respond to review labels May 21, 2021
@codecov

codecov Bot commented May 22, 2021

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.06@0ebf7e6). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #8256   +/-   ##
===============================================
  Coverage                ?   82.88%           
===============================================
  Files                   ?      105           
  Lines                   ?    17874           
  Branches                ?        0           
===============================================
  Hits                    ?    14814           
  Misses                  ?     3060           
  Partials                ?        0           

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 0ebf7e6...2f2db02. Read the comment docs.

@harrism

harrism commented May 24, 2021

Copy link
Copy Markdown
Contributor

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit c398054 into NVIDIA:branch-21.06 May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants