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

Support Scatter struct_scalar #8630

Merged
merged 11 commits into from
Jul 9, 2021

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Jun 30, 2021

Partially addresses #8558

This PR adds support for scattering struct scalars.

Implementation notes:
Current implementation is based on copying the row data from each field as a new scalar and recursively calls scalar scattering on each field. There maybe an optimization on eliminating such copy. But will require extra scaffolding/scatter machinery.

Minor aspects of this PR:

  • Refactors column_scalar_scatterer to include scatter_scalar_bitmask_inplace in each level of dispatch. This is required because scalar scattering can be nested.
  • Adds count_set_bit and count_unset_bit detail APIs
  • Adds default stream/mr for detail::get_element

@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Jun 30, 2021
@isVoid isVoid changed the title Support Scatter Struct Scalar Support Scatter struct_scalar Jun 30, 2021
@isVoid isVoid added feature request New feature or request non-breaking Non-breaking change labels Jun 30, 2021
@isVoid isVoid marked this pull request as ready for review June 30, 2021 21:49
@isVoid isVoid requested a review from a team as a code owner June 30, 2021 21:49
@codecov
Copy link

codecov bot commented Jul 1, 2021

Codecov Report

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

❗ Current head a05607f differs from pull request most recent head 199eccf. Consider uploading reports for the commit 199eccf to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8630   +/-   ##
===============================================
  Coverage                ?   10.64%           
===============================================
  Files                   ?      109           
  Lines                   ?    18656           
  Branches                ?        0           
===============================================
  Hits                    ?     1985           
  Misses                  ?    16671           
  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 b123eb8...199eccf. Read the comment docs.

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

Looks awesome! Only a couple small comments

cpp/src/copying/scatter.cu Outdated Show resolved Hide resolved
cpp/src/copying/scatter.cu Outdated Show resolved Hide resolved
cpp/src/copying/scatter.cu Outdated Show resolved Hide resolved
@isVoid
Copy link
Contributor Author

isVoid commented Jul 7, 2021

rerun tests

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

One small change, looks great other than that!

cpp/src/copying/scatter.cu Outdated Show resolved Hide resolved
@isVoid
Copy link
Contributor Author

isVoid commented Jul 8, 2021

rerun tests

Copy link
Contributor

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

Verified that gerashegalov#1 works on top of this PR

@isVoid
Copy link
Contributor Author

isVoid commented Jul 9, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8bd0dfe into rapidsai:branch-21.08 Jul 9, 2021
rapids-bot bot pushed a commit that referenced this pull request Jul 20, 2021
Uses scalar-vector-based scatter API to provide support for copy_if_else involving scalar columns. 

Other changes:
- removes some dead code
- refactoring into overloaded functions

Closes #8361, depends on #8630, #8666

Authors:
  - Gera Shegalov (https://github.com/gerashegalov)

Approvers:
  - https://github.com/nvdbaranec
  - MithunR (https://github.com/mythrocks)

URL: #8588
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants