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

[REVIEW] Add dictionary support to cudf::minmax #6764

Merged
merged 10 commits into from
Nov 20, 2020

Conversation

davidwendt
Copy link
Contributor

Reference #5963

Add support for dictionary column types in cudf::minmax. This PR also adds support for strings column types as well. The code was refactored to re-use the internal reduce_device function for all supported types. The strings specialization logic just creates a string_scalar result which requires a copy step to build a new string_scalar object from a string_view (output from the reduce_device function). The dictionary support was to just to substitute the keys() column in the detail function right before the type-dispatcher call.

Added a gtests for strings as well as dictionary with strings keys.

@davidwendt davidwendt requested a review from a team as a code owner November 13, 2020 19:24
@davidwendt davidwendt self-assigned this Nov 13, 2020
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. labels Nov 13, 2020
@codecov
Copy link

codecov bot commented Nov 13, 2020

Codecov Report

Merging #6764 (cc8e21d) into branch-0.17 (8c420ae) will increase coverage by 0.71%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.17    #6764      +/-   ##
===============================================
+ Coverage        81.71%   82.42%   +0.71%     
===============================================
  Files               96       96              
  Lines            15978    16843     +865     
===============================================
+ Hits             13056    13883     +827     
- Misses            2922     2960      +38     
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/column/column.py 87.70% <0.00%> (+0.10%) ⬆️
python/cudf/cudf/core/series.py 92.80% <0.00%> (+1.54%) ⬆️
python/cudf/cudf/utils/utils.py 87.82% <0.00%> (+3.93%) ⬆️
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 8c420ae...cc8e21d. Read the comment docs.

devavret
devavret previously approved these changes Nov 16, 2020
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

Just realized something. Not requesting changes because need confirmation if what I'm thinking is correct.

cpp/src/reductions/minmax.cu Outdated Show resolved Hide resolved
@devavret devavret dismissed their stale review November 16, 2020 10:14

Caught something after the review

@davidwendt davidwendt requested a review from devavret November 17, 2020 22:30
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

One non-dealbreaker comment but approve otherwise.

cpp/src/reductions/minmax.cu Outdated Show resolved Hide resolved
@harrism harrism merged commit de5577c into rapidsai:branch-0.17 Nov 20, 2020
@davidwendt davidwendt deleted the dictionary-minmax branch November 20, 2020 18:39
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 libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants