Fixed issue with percentile_approx where output tdigests could have uninitialized data at the end. #9931
Conversation
nvdbaranec
added
bug
Codecov Report
@@ Coverage Diff @@
## branch-22.02 #9931 +/- ##
================================================
- Coverage 10.49% 10.42% -0.07%
================================================
Files 119 119
Lines 20305 20475 +170
================================================
+ Hits 2130 2134 +4
- Misses 18175 18341 +166
Continue to review full report at Codecov.
|
|
I have tested this patch locally with the RAPIDS Accelerator tests that were originally failing and I have not seen any failures. |
sameerz
changed the title
|
rerun tests |
| auto const group_start = inner_offsets[outer_offsets[group_index]]; | ||
| auto const group_end = inner_offsets[outer_offsets[group_index + 1]]; | ||
| auto const num_weights = group_end - group_start; | ||
| auto const last_weight_index = group_end - 1; |
There was a problem hiding this comment.
Minor nit: You could inline this so that you online calculate it when the calculation is false. Should be negligible though since I assume num_weights is almost always nonzero and you're already avoiding actually indexing into cumulative_weights except when needed.
There was a problem hiding this comment.
I'm pretty sure the compiler will optimize this correctly, and I think group_end - 1 is a bit on the cryptic side. This code is already drilling through 2 layers of offsets which is confusing to begin with :)
| double total_weight; | ||
| size_type group_size, group_start; | ||
| thrust::tie(total_weight, group_size, group_start) = group_info(group_index); |
There was a problem hiding this comment.
I assume you need a tie here because of thrust::tuple not supporting structured bindings?
There was a problem hiding this comment.
Correct. Same as
cudf/cpp/src/groupby/sort/group_tdigest.cu
Line 349 in fbf769f
|
@gpucibot merge |
Fixes NVIDIA/spark-rapids#4060
Issue was relatively straightforward. There is a section of code in the bucket generation step that detects "gaps" that would be generated during the reduction step. It was incorrectly indexing into the list of cumulative weights for input values. Fundamental change was to change the
TotalWeightIteriterator which was just returning the total weight for an input group into aGroupInfoFuncfunctor that returns total weight as well as group size info that is used to index cumulative weights correctly.