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

Implement cudf::reduce for decimal32 and decimal64 (part 2) #6980

Merged
merged 10 commits into from
Dec 17, 2020

Conversation

codereport
Copy link
Contributor

@codereport codereport commented Dec 11, 2020

This PR resolves a part of #3556.

Supporting cudf::reduce:

  1. Part 1 (MIN, MAX, SUM & PRODUCT & NUNIQUE) Implement cudf::reduce for decimal32 and decimal64 (part 1) #6814
  2. Part 2 (the rest) ◀️

Reduction Ops:

Done in Previous PR
✔️ SUM, ///< sum reduction
✔️ PRODUCT, ///< product reduction
✔️ MIN, ///< min reduction
✔️ MAX, ///< max reduction
✔️ NUNIQUE, ///< count number of unique elements

Not supported by cudf::reduce:

  • COUNT_VALID, ///< count number of valid elements
  • COUNT_ALL, ///< count number of elements
  • COLLECT, ///< collect values into a list
  • LEAD, ///< window function, accesses row at specified offset following current row
  • LAG, ///< window function, accesses row at specified offset preceding current row
  • PTX, ///< PTX UDF based reduction
  • CUDA ///< CUDA UDf based reduction
  • ARGMAX, ///< Index of max element
  • ARGMIN, ///< Index of min element
  • ROW_NUMBER, ///< get row-number of element

Won't be supported:

  • ANY, ///< any reduction
  • ALL, ///< all reduction

To Do / Investigate:

  • SUM_OF_SQUARES, ///< sum of squares reduction
  • MEDIAN, ///< median reduction
  • QUANTILE, ///< compute specified quantile(s)
  • NTH_ELEMENT, ///< get the nth element

Deferred until requested

  • MEAN, ///< arithmetic mean reduction
  • VARIANCE, ///< groupwise variance
  • STD, ///< groupwise standard deviation

@codereport codereport added feature request New feature or request 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Dec 11, 2020
@codereport codereport self-assigned this Dec 11, 2020
@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #6980 (2bf3675) into branch-0.18 (7ca3fad) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #6980      +/-   ##
===============================================
+ Coverage        82.04%   82.06%   +0.02%     
===============================================
  Files               96       96              
  Lines            16388    16391       +3     
===============================================
+ Hits             13446    13452       +6     
+ Misses            2942     2939       -3     
Impacted Files Coverage Δ
python/cudf/cudf/_fuzz_testing/fuzzer.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/hash_vocab_utils.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 91.48% <0.00%> (+4.25%) ⬆️
python/cudf/cudf/utils/gpu_utils.py 58.53% <0.00%> (+4.87%) ⬆️

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 7ca3fad...2bf3675. Read the comment docs.

@codereport codereport added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond libcudf Affects libcudf (C++/CUDA) code. and removed 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. labels Dec 15, 2020
@codereport codereport marked this pull request as ready for review December 15, 2020 03:56
@codereport codereport requested a review from a team as a code owner December 15, 2020 03:56
Copy link
Contributor

@nvdbaranec nvdbaranec left a comment

Choose a reason for hiding this comment

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

One very minor (and certainly debatable) suggestion.

cpp/src/copying/get_element.cu Show resolved Hide resolved
cpp/src/quantiles/quantile.cu Outdated Show resolved Hide resolved

return std::make_unique<fixed_point_scalar<T>>(std::move(temp_data),
numeric::scale_type{input.type().scale()},
temp_valid.value(stream),
Copy link
Contributor

@karthikeyann karthikeyann Dec 16, 2020

Choose a reason for hiding this comment

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

Just wondering if it makes sense to add scalar constructor with rmm::device_scalar<bool>&& valid argument. (not in the scope of this PR)
std::move(temp_valid).
It would avoid extra DtoH and HtoD copy of single bool.

@davidwendt
Copy link
Contributor

rerun tests

2 similar comments
@codereport
Copy link
Contributor Author

rerun tests

@mike-wendt
Copy link
Contributor

rerun tests

@codereport codereport added 6 - Okay to Auto-Merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Dec 16, 2020
@rapids-bot rapids-bot bot merged commit ae17c14 into rapidsai:branch-0.18 Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants