-
Notifications
You must be signed in to change notification settings - Fork 891
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
Allow initial value for cudf::reduce and cudf::segmented_reduce. #11137
Conversation
Codecov Report
@@ 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.
|
Pinging @bdice |
There was a problem hiding this 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).
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
There was a problem hiding this 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.
There was a problem hiding this 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.
@gpucibot merge |
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
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