-
Notifications
You must be signed in to change notification settings - Fork 915
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
Add compound aggregations to cudf::segmented_reduce #12573
Add compound aggregations to cudf::segmented_reduce #12573
Conversation
This will target 23.04 once it is ready. |
@@ -229,92 +228,6 @@ std::unique_ptr<scalar> reduce(InputIterator d_in, | |||
return std::unique_ptr<scalar>(result); | |||
} | |||
|
|||
/** |
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.
These functions were moved to segmented_reduction.cuh
@@ -338,171 +338,5 @@ std::unique_ptr<scalar> merge_sets( | |||
rmm::cuda_stream_view stream, | |||
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource()); | |||
|
|||
/** |
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.
These functions were moved to segmented_reduction_functions.hpp
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## branch-23.04 #12573 +/- ##
===============================================
Coverage ? 85.81%
===============================================
Files ? 158
Lines ? 25153
Branches ? 0
===============================================
Hits ? 21586
Misses ? 3567
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…cudf into reduction-segmented-compound
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.
CMake changes LGTM
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.
Approving ops-codeowner
file changes
…cudf into reduction-segmented-compound
…cudf into reduction-segmented-compound
…cudf into reduction-segmented-compound
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.
Reviewed with @davidwendt. A few comments attached. Great work!
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.
Holding off for a final review pass.
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.
Great! Thanks @davidwendt.
/merge |
Reworks some internal source specific to fixed-point types using `cudf::reduce` by removing the duplicated code logic. This was found while working on #12573 and #10432. Since the fix is requires no dependencies, this separate PR is used to minimize code review churn. This should help with code consistency with the fixed-point-specific logic when added to segmented-reduction. No function has changed so all existing gtests are adequate. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Yunsong Wang (https://github.com/PointKernel) - Bradley Dice (https://github.com/bdice) URL: #12652
Depends on #12573 Adds additional support for fixed-point types in `cudf::segmented_reduce` for simple aggregations: sum, product, and sum-of-squares. Reference: #10432 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Mike Wilson (https://github.com/hyperbolic2346) - Nghia Truong (https://github.com/ttnghia) - Bradley Dice (https://github.com/bdice) URL: #12680
Description
Adds mean, variance, and standard deviation aggregation support to
cudf::segmented_reduce
. These are compound (multi-step) aggregations and are modeled after the same aggregations supported butcudf::reduce
. Once this approved and merged, the visitor pattern for this approach will be reworked for bothcudf::reduce
andcudf::segmented_reduce
as per #10432.The source tree for
src/reductions
has been adjusted to put all segmented-reduce source files intosrc/reductions/segmented
and removing thesegmented_
prefix from those file names.Also, the segmented-reduce functions have been moved from
cudf/detail/reduction_functions.hpp
into their owncudf/detail/segmented_reduction_functions.hpp
. Likewise, the segmented-reduce CUB calls have been moved fromcudf/detail/reduction.cuh
to the newcudf/detail/segmented_reduction.cuh
to help minimize including CUB headers.Additionally, the sum-of-squares aggregation is also included since it was a simple reduction only requiring the appropriate aggregation class registration and source file.
Finally, gtests are added for these new types. The compound types only support floating-point outputs.
Follow on PRs will address the visitor pattern already mentioned above as well as additional data types. Discussion on additional aggregations will occur in the reference issue #10432.
Checklist