-
Notifications
You must be signed in to change notification settings - Fork 886
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
Fix call to thrust::reduce_by_key in argmin/argmax libcudf groupby #9244
Fix call to thrust::reduce_by_key in argmin/argmax libcudf groupby #9244
Conversation
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #9244 +/- ##
================================================
- Coverage 10.85% 10.84% -0.01%
================================================
Files 115 116 +1
Lines 19158 19171 +13
================================================
Hits 2080 2080
- Misses 17078 17091 +13
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be branched from 21.12 but targeting 21.10, you will need to rebase on 21.10
…t/cudf into bug-sorted-groupby-min-strings
I seemed to have hosed my branch now. I will close the PR and reopen based on 21.10 |
…9263) Closes #9156 This PR simplifies the parameters when calling thrust::reduce_by_key for the argmin/argmax aggregations in cudf::groupby. The illegalMemoryAccess found in #9156 was due to invalid data being passed from thrust::reduce_by_key through to the BinaryPredicate function as documented in NVIDIA/thrust#1525 The invalid data being passed is only a real issue for strings columns where the device pointer was neither nullptr nor a valid address. The new logic provides only size_type values to thrust::reduce_by_key so invalid values can only be out-of-bounds for the input column which is easily checked before retrieving the string_view objects within the ArgMin and ArgMax operators. This the same as #9244 but based on 21.10 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Devavret Makkar (https://github.com/devavret) - Nghia Truong (https://github.com/ttnghia) - Robert Maynard (https://github.com/robertmaynard) URL: #9263
Closes #9156
This PR simplifies the parameters when calling
thrust::reduce_by_key
for the argmin/argmax aggregations in cudf::groupby. TheillegalMemoryAccess
found in #9156 was due to invalid data being passed fromthrust::reduce_by_key
through to the BinaryPredicate function as documented in NVIDIA/cccl#789The invalid data being passed is only a real issue for strings columns where the device pointer was neither nullptr nor a valid address. The new logic provides only
size_type
values tothrust::reduce_by_key
so invalid values can only be out-of-bounds for the input column which is easily checked before retrieving thestring_view
objects within the ArgMin and ArgMax operators.