-
Notifications
You must be signed in to change notification settings - Fork 196
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
Clean up merge_by_key
and merge_key_value
tests
#1824
Clean up merge_by_key
and merge_key_value
tests
#1824
Conversation
merge_by_key
and merge_key_value
tests
95c3a71
to
f1f1866
Compare
f1f1866
to
05934a7
Compare
_CCCL_IF_CONSTEXPR (::cuda::std::is_void<CompareOp>::value) | ||
{ | ||
return thrust::merge_by_key(std::forward<Args>(args)...); | ||
} | ||
else | ||
{ | ||
// TODO(bgruber): remove next line in C++17 and pass CompareOp{} directly to stable_sort | ||
using C = ::cuda::std::__conditional_t<::cuda::std::is_void<CompareOp>::value, thrust::less<T>, CompareOp>; | ||
return thrust::merge_by_key(std::forward<Args>(args)..., C{}); | ||
} |
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 a bit annoying to handle, but I want to test calling the thrust API with and without a comparison functor. C++11 makes this additionally hard, since it wants to compile both branches of the if constexpr
.
🟨 CI finished in 2h 00m: Pass: 98%/249 | Total: 1d 09h | Avg: 8m 09s | Max: 53m 11s | Hits: 97%/245757
|
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
+/- | Thrust |
CUDA Experimental |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
+/- | CUB |
+/- | Thrust |
CUDA Experimental |
🏃 Runner counts (total jobs: 249)
# | Runner |
---|---|
178 | linux-amd64-cpu16 |
40 | linux-amd64-gpu-v100-latest-1 |
16 | linux-arm64-cpu16 |
15 | windows-amd64-cpu16 |
🟩 CI finished in 3h 26m: Pass: 100%/249 | Total: 1d 10h | Avg: 8m 20s | Max: 53m 11s | Hits: 97%/248310
|
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
+/- | Thrust |
CUDA Experimental |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
+/- | CUB |
+/- | Thrust |
CUDA Experimental |
🏃 Runner counts (total jobs: 249)
# | Runner |
---|---|
178 | linux-amd64-cpu16 |
40 | linux-amd64-gpu-v100-latest-1 |
16 | linux-arm64-cpu16 |
15 | windows-amd64-cpu16 |
ref_key[5] = 4; ref_val[5] = 42; | ||
ref_key[6] = 4; ref_val[6] = 13; | ||
// clang-format on | ||
const Vector a_key{0, 2, 4}, a_val{13, 7, 42}, b_key{0, 3, 3, 4}, b_val{42, 42, 7, 13}; |
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.
Important: We have never written it out, but AFAIK we strongly encourage one line per variable
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.
There are a lot of tests written like that. I will keep it in mind for future PRs!
thrust::host_vector<T> h_a_keys(random_keys.begin(), random_keys.begin() + size_a); | ||
thrust::host_vector<T> h_b_keys(random_keys.begin() + size_a, random_keys.end()); | ||
|
||
thrust::host_vector<T> h_a_vals(random_vals.begin(), random_vals.begin() + size_a); | ||
thrust::host_vector<T> h_b_vals(random_vals.begin() + size_a, random_vals.end()); | ||
const thrust::host_vector<T> h_a_vals(random_vals.begin(), random_vals.begin() + size_a); | ||
const thrust::host_vector<T> h_b_vals(random_vals.begin() + size_a, random_vals.end()); |
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.
I am wondering, we are always just sorting subranges of the same vector. Could we just use a cuda::std::span
and avoid the separate vectors?
Just kidding we still have C++11
my_system() = delete; | ||
|
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.
Nitpick: I believe that is superfluous
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.
Correct!
Spin-off from porting
thrust::merge
to CUB (#1817)