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

Add groupby nunique aggregation benchmark #11472

Merged
merged 25 commits into from
Aug 8, 2022

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Aug 4, 2022

This adds a simple benchmark for groupby nunique aggregation.

@ttnghia ttnghia added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 4, 2022
@ttnghia ttnghia self-assigned this Aug 4, 2022
@ttnghia ttnghia added this to PR-WIP in v22.10 Release via automation Aug 4, 2022
@ttnghia ttnghia requested a review from a team as a code owner August 4, 2022 20:22
@ttnghia ttnghia requested review from upsj and bdice August 4, 2022 20:22
@ttnghia ttnghia moved this from PR-WIP to PR-Needs review in v22.10 Release Aug 4, 2022
@github-actions github-actions bot added the CMake CMake build issue label Aug 4, 2022
Signed-off-by: Nghia Truong <nghiatruong.vn@gmail.com>
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

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

@@               Coverage Diff               @@
##             branch-22.10   #11472   +/-   ##
===============================================
  Coverage                ?   86.47%           
===============================================
  Files                   ?      144           
  Lines                   ?    22856           
  Branches                ?        0           
===============================================
  Hits                    ?    19765           
  Misses                  ?     3091           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

One comment, otherwise LGTM!

Comment on lines 60 to 62
requests.emplace_back(cudf::groupby::aggregation_request());
requests[0].values = vals;
requests[0].aggregations.push_back(cudf::make_nunique_aggregation<cudf::groupby_aggregation>());
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like we should initialize the aggregation request with the data, rather than create-and-modify the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately initializing the request class can't be done by initializer list because it has a unique_ptr member. We have to call emplace_back/push_back anyway :(

v22.10 Release automation moved this from PR-Needs review to PR-Reviewer approved Aug 4, 2022
Signed-off-by: Nghia Truong <nghiatruong.vn@gmail.com>
Copy link
Contributor

@upsj upsj left a comment

Choose a reason for hiding this comment

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

LGTM!

@ttnghia
Copy link
Contributor Author

ttnghia commented Aug 8, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6b20f2a into rapidsai:branch-22.10 Aug 8, 2022
v22.10 Release automation moved this from PR-Reviewer approved to Done Aug 8, 2022
@ttnghia ttnghia deleted the add_benchmark branch August 8, 2022 13:20
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 CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants