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

Allow initial value for cudf::reduce and cudf::segmented_reduce. #11137

Merged

Conversation

SrikarVanavasam
Copy link
Contributor

@SrikarVanavasam SrikarVanavasam commented Jun 22, 2022

Closes #11002

Previously, the algorithms cudf::reduce and cudf::segmented_reduce did not accept an initial value. This PR overloads of the algorithms that do accept an initial value. Initial values are not yet supported for segmented string reductions and can be addressed in a second PR

@SrikarVanavasam SrikarVanavasam requested a review from a team as a code owner June 22, 2022 22:01
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 22, 2022
@SrikarVanavasam SrikarVanavasam added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function 2 - In Progress Currently a work in progress and removed 3 - Ready for Review Ready for review by team labels Jun 22, 2022
@SrikarVanavasam SrikarVanavasam added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jun 22, 2022
@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

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

@@               Coverage Diff               @@
##             branch-22.08   #11137   +/-   ##
===============================================
  Coverage                ?   86.31%           
===============================================
  Files                   ?      144           
  Lines                   ?    22748           
  Branches                ?        0           
===============================================
  Hits                    ?    19636           
  Misses                  ?     3112           
  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 002cb1c...3e87dcb. Read the comment docs.

@karthikeyann karthikeyann added the non-breaking Non-breaking change label Jun 23, 2022
@nvdbaranec
Copy link
Contributor

Pinging @bdice

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.

First round of feedback attached. This is great overall! I have a lot of suggestions but this is aligned with our previous design discussions, which is fantastic (and makes me thankful that we spent the time discussing/planning).

cpp/include/cudf/detail/null_mask.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction_functions.hpp Outdated Show resolved Hide resolved
@@ -93,13 +99,15 @@ std::unique_ptr<scalar> max(
*
* @param col input column to compute any_of.
* @param output_dtype data type of return type and typecast elements of input column
* @param init initial value of the any
Copy link
Contributor

Choose a reason for hiding this comment

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

At some later time, we might consider short-circuiting execution for any/all if the initial value is true/false, respectively. No need to look at the data -- but that might be a premature optimization at this point.

cpp/src/reductions/simple.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/reduction.hpp Outdated Show resolved Hide resolved
cpp/src/reductions/simple.cuh Outdated Show resolved Hide resolved
cpp/src/reductions/simple.cuh Outdated Show resolved Hide resolved
cpp/src/reductions/simple.cuh Outdated Show resolved Hide resolved
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.

Comments attached. I'll do a final round of review soon - this is looking pretty good!

cpp/include/cudf/detail/null_mask.cuh Show resolved Hide resolved
cpp/include/cudf/detail/null_mask.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/null_mask.cuh Show resolved Hide resolved
cpp/include/cudf/detail/null_mask.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction_functions.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/reduction.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/reduction.hpp Outdated Show resolved Hide resolved
cpp/src/reductions/simple.cuh Outdated Show resolved Hide resolved
cpp/src/reductions/simple_segmented.cuh Outdated Show resolved Hide resolved
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.

This is getting very close. I have one more round of feedback and I've marked all the files as viewed. I'll do a final review of what changes from this round of feedback, then approve.

cpp/include/cudf/detail/reduction.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction_functions.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction.cuh Show resolved Hide resolved
cpp/src/reductions/segmented_reductions.cpp Outdated Show resolved Hide resolved
cpp/tests/reductions/reduction_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/reductions/reduction_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/reductions/reduction_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/reductions/reduction_tests.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

Also should merge with the latest commit from origin.

@nvdbaranec
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 149bbd0 into rapidsai:branch-22.08 Jul 13, 2022
rapids-bot bot pushed a commit that referenced this pull request Jul 13, 2022
The recently merged PR (#11137) did not include the `<optional>` header which may cause compile error in some systems (in particular, CUDA 11.7 + gcc-11.2):
```
error: ‘std::optional’ has not been declared
error: ‘optional’ in namespace ‘std’ does not name a template type
```

This PR adds that missing header to fix the compile issue.

Authors:
  - Nghia Truong (https://github.com/ttnghia)

Approvers:
  - Gera Shegalov (https://github.com/gerashegalov)
  - David Wendt (https://github.com/davidwendt)
  - Mike Wilson (https://github.com/hyperbolic2346)

URL: #11257
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Allow initial value for cudf::reduce and cudf::segmented_reduce.
8 participants