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.

Sign up for GitHub

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

Jump to bottom

Segmented Min/Max for Fixed Point Types #10794

Merged
merged 17 commits into from
May 18, 2022
Merged

Segmented Min/Max for Fixed Point Types #10794

merged 17 commits into from
May 18, 2022

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented May 4, 2022 •
edited
Loading

This PR adds support to min/max segmented reduction to fixed point type. Together with #10447, this PR closes #10417

Besides, this PR refactors segmented_reduce to accept output iterators instead of allocating the result column from within.

@isVoid isVoid added feature request New feature or request 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels May 4, 2022
@isVoid isVoid requested a review from bdice May 4, 2022 23:11
@isVoid isVoid requested a review from a team as a code owner May 4, 2022 23:11
@isVoid isVoid self-assigned this May 4, 2022
@isVoid isVoid requested a review from rgsl888prabhu May 4, 2022 23:11
@codecov
Copy link

codecov bot commented May 5, 2022 •
edited
Loading

Codecov Report

Merging #10794 (9fbaf2c) into branch-22.06 (8d861ce) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.06   #10794      +/-   ##
================================================
- Coverage         86.40%   86.32%   -0.08%     
================================================
  Files               143      144       +1     
  Lines             22448    22656     +208     
================================================
+ Hits              19396    19558     +162     
- Misses             3052     3098      +46
Impacted Files Coverage Δ
...thon/dask_cudf/dask_cudf/tests/test_distributed.py 18.86% <0.00%> (-67.93%) ⬇️
python/cudf/cudf/io/parquet.py 90.83% <0.00%> (-1.87%) ⬇️
python/dask_cudf/dask_cudf/backends.py 85.51% <0.00%> (-0.94%) ⬇️
python/cudf/cudf/core/column/categorical.py 89.37% <0.00%> (-0.61%) ⬇️
python/cudf/cudf/core/column/decimal.py 90.60% <0.00%> (-0.50%) ⬇️
python/cudf/cudf/core/column/lists.py 91.70% <0.00%> (-0.38%) ⬇️
python/cudf/cudf/core/scalar.py 89.01% <0.00%> (-0.31%) ⬇️
python/cudf/cudf/core/column/string.py 88.78% <0.00%> (-0.31%) ⬇️
python/cudf/cudf/core/index.py 92.06% <0.00%> (-0.25%) ⬇️
python/dask_cudf/dask_cudf/io/tests/test_s3.py 96.05% <0.00%> (-0.15%) ⬇️
... and 23 more

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 8d861ce...9fbaf2c. Read the comment docs.

bdice
bdice requested changes May 6, 2022
View reviewed changes
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.

I have some suggestions attached. Overall this looks pretty good!

cpp/include/cudf/detail/reduction.cuh Outdated Show resolved Hide resolved
cpp/src/reductions/simple_segmented.cuh Outdated Show resolved Hide resolved
cpp/src/reductions/simple_segmented.cuh Show resolved Hide resolved
@isVoid isVoid requested a review from bdice May 11, 2022 01:06
bdice
bdice approved these changes May 12, 2022
View reviewed changes
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.

A couple small suggestions, otherwise LGTM!

cpp/include/cudf/detail/reduction.cuh Show resolved Hide resolved
cpp/include/cudf/detail/reduction.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/reduction.cuh Show resolved Hide resolved
@isVoid
Merge branch 'fea/segmented_reduction_decimal_minmax' of github.com:i…
9fbaf2c 
…sVoid/cudf into fea/segmented_reduction_decimal_minmax
@bdice bdice requested a review from davidwendt May 16, 2022 19:56
@isVoid isVoid added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels May 17, 2022
@isVoid
Copy link
Contributor Author

isVoid commented May 18, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit dee435f into rapidsai:branch-22.06 May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@rgsl888prabhu rgsl888prabhu rgsl888prabhu approved these changes

@bdice bdice bdice approved these changes

Assignees

@isVoid isVoid

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] min/max segmented reduce for string and decimal
4 participants
@isVoid @bdice @rgsl888prabhu @davidwendt