-
Notifications
You must be signed in to change notification settings - Fork 907
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 exclusive scan when including nulls and improve testing #8478
Fix exclusive scan when including nulls and improve testing #8478
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #8478 +/- ##
===============================================
Coverage ? 83.00%
===============================================
Files ? 109
Lines ? 18215
Branches ? 0
===============================================
Hits ? 15119
Misses ? 3096
Partials ? 0 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.
LGTM
cpp/include/cudf_test/type_lists.hpp
Outdated
@@ -85,15 +86,21 @@ typename std::enable_if<cudf::is_fixed_width<TypeParam>() && | |||
make_type_param_vector(std::initializer_list<T> const& init_list) | |||
{ | |||
thrust::host_vector<TypeParam> vec(init_list.size()); | |||
std::transform(std::cbegin(init_list), std::cend(init_list), std::begin(vec), [](auto const& e) { | |||
if (std::is_unsigned<TypeParam>::value) | |||
std::transform(std::cbegin(init_list), std::cend(init_list), std::begin(vec), [](T const& e) { |
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 is interesting. Why did we need the T
to be explicit?
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.
We don't. Fixed, thanks. Also changed from if constexpr {return} else {return}
to if constexpr {return} return
to workaround known CUDA compiler issue (spurious warning I just noticed).
@gpucibot merge |
Fixes #8462 by generalizing the existing
mask_inclusive_scan
used ininclusive_scan
so it can be applied the same way inexclusive_scan
. #8462 demonstrated holes in test coverage, so this PR also reorganizesscan_tests
test infrastructure to enable a single set of tests to be applied to all supported data types and parameters, correctly expecting exceptions in unsupported cases (e.g. no product scans on fixed-point, only min/max inclusive scans on strings).