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

[FEA] Refactors and next steps for segmented reductions #10432

Open
bdice opened this issue Mar 14, 2022 · 6 comments
Open

[FEA] Refactors and next steps for segmented reductions #10432

bdice opened this issue Mar 14, 2022 · 6 comments
Assignees
Labels
0 - Backlog In queue waiting for assignment feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.

Comments

@bdice
Copy link
Contributor

bdice commented Mar 14, 2022

This is issue contains a few proposals for improving the segmented reduction code introduced in #9621.

Investigate sort-groupby aggregations

(Idea from @ttnghia)

With the ability to perform segmented reductions, sort-based groupby may be able to use group_offsets to define its segments, rather than materializing a full column of sorted/monotonic group_labels. In effect, this allows us to replace a call to thrust::reduce_by_key algorithm with a call to cub::DeviceSegmentedReduce::Reduce, while eliminating the need to compute the group_labels column. I think this should be a more efficient algorithm, and also will require less intermediate memory allocation. Benchmarks should be performed when making this change.

Refactor internal use of indices to 2N style (match CUB)

The indexing scheme used for segmented reduction is currently "N+1", like how list offsets are indexed. We want to refactor this to use "2N" indexing. This would align with cub::DeviceSegmentedReduce::Reduce and permit greater flexibility in the API internals. See discussion here for details.

  • @bdice: After discussion with @davidwendt, we decided this is not necessary in the short term. We can resolve this issue without changing the current implementation. The current implementation of N+1 aligns with segmented sort behavior. If there is a compelling need to change this in the future for expanded functionality, we can revisit.

Compound reductions like mean

The segmented reduction code currently supports "simple" reductions. Support for "compound" reductions is needed. This includes multi-step calculations like mean, standard deviation, or sum of squares. Non-segmented compound reductions are defined here: https://github.com/rapidsai/cudf/blob/c1638869116aae2c6dde6024394279a2fb79e685/cpp/src/reductions/compound.cuh

Fixes for output_type precision

@isVoid and I filed #9988 while working on #9621 because the documentation doesn't align with the implementation for when data is cast to the output dtype relative to when the reduction is performed. This affects segmented reduction as well.

Explore rewriting get_null_replacing_element_transformer with nullate

It may be possible to clean up the implementation of null element handling here by using nullate.

  • @bdice: After discussion with @davidwendt, we decided this is not worth changing. It might eliminate one or two lines of duplicate code but doesn't offer any benefits to compile time.

Extend to more data types

We need to review the types supported by non-segmented reductions and ensure that segmented reductions support the same types. Decimal support has been requested here: #10417

@bdice bdice added the feature request New feature or request label Mar 14, 2022
@jrhemstad
Copy link
Contributor

Support for "compound" reductions is needed.

I'd suggest moving segmented (and regular) reductions to use a scheme like in groupby/rolling where every aggregation has a preprocessing/finalization steps:

std::vector<std::unique_ptr<aggregation>> get_simple_aggregations(
data_type col_type, simple_aggregations_collector& collector) const override
{
return collector.visit(col_type, *this);
}
void finalize(aggregation_finalizer& finalizer) const override { finalizer.visit(*this); }

@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@isVoid
Copy link
Contributor

isVoid commented Apr 20, 2022

Still in development.

@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@GregoryKimball GregoryKimball added libcudf Affects libcudf (C++/CUDA) code. 0 - Backlog In queue waiting for assignment and removed inactive-30d labels Nov 21, 2022
rapids-bot bot pushed a commit that referenced this issue Dec 2, 2022
Fixes reduction gtests source files coded in namespace `cudf::test`
No function or test has changed just the source code reworked per namespaces.

Fixing this ahead of any changes for #10432 
Reference #11734

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Bradley Dice (https://github.com/bdice)
  - MithunR (https://github.com/mythrocks)

URL: #12257
rapids-bot bot pushed a commit that referenced this issue Dec 5, 2022
Fixes replace gtests source files coded in namespace `cudf::test`
This only required fixing `replace_nans_tests.cpp`
No function or test has changed just the test source code reworked per namespaces.

Fixing this ahead of any changes for #10432
Reference #11734

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Mike Wilson (https://github.com/hyperbolic2346)
  - Nghia Truong (https://github.com/ttnghia)

URL: #12270
rapids-bot bot pushed a commit that referenced this issue Jan 11, 2023
This removes/updates some `TODO` comments from the code after discussions on issue #10432 with @davidwendt.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - David Wendt (https://github.com/davidwendt)
  - Nghia Truong (https://github.com/ttnghia)

URL: #12528
@davidwendt davidwendt self-assigned this Jan 13, 2023
@GregoryKimball GregoryKimball moved this to Pairing in libcudf Jan 15, 2023
@bdice
Copy link
Contributor Author

bdice commented Feb 2, 2023

After merging #12573, we can update this to use segmented reductions detail APIs like cudf::reduction::detail::segmented_reduce:

CUDF_CUDA_TRY(cub::DeviceSegmentedReduce::Sum(nullptr,

rapids-bot bot pushed a commit that referenced this issue Feb 3, 2023
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 but `cudf::reduce`. Once this approved and merged, the visitor pattern for this approach will be reworked for both `cudf::reduce` and `cudf::segmented_reduce` as per [#10432](#10432 (comment)).

The source tree for `src/reductions` has been adjusted to put all segmented-reduce source files into `src/reductions/segmented` and removing the `segmented_` prefix from those file names.
Also, the segmented-reduce functions have been moved from `cudf/detail/reduction_functions.hpp` into their own `cudf/detail/segmented_reduction_functions.hpp`. Likewise, the segmented-reduce CUB calls have been moved from `cudf/detail/reduction.cuh` to the new `cudf/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.

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Mike Wilson (https://github.com/hyperbolic2346)
  - Bradley Dice (https://github.com/bdice)

URL: #12573
rapids-bot bot pushed a commit that referenced this issue Feb 7, 2023
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
rapids-bot bot pushed a commit that referenced this issue Feb 23, 2023
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
rapids-bot bot pushed a commit that referenced this issue Mar 27, 2023
Adds support for `NUNIQUE` aggregation type for `cudf::segmented_reduce`. This computes the number of unique elements within each segment specified. Due to the overhead of sorting, the segments must be sorted before calling this function otherwise the results are undefined. Also, only non-nested column types are supported as well.

Reference #10432

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Divye Gala (https://github.com/divyegala)
  - Karthikeyan (https://github.com/karthikeyann)
  - Bradley Dice (https://github.com/bdice)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #12972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Backlog In queue waiting for assignment feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.
Projects
Status: Pairing
Development

No branches or pull requests

5 participants