[STF] Move unstable_unique from STF to generic cudax utility#8190
[STF] Move unstable_unique from STF to generic cudax utility#8190caugonnet wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
unstable_unique is a generic algorithm with no STF dependency. Move it to cuda::experimental namespace under __utility/, replace the STF UNITTEST with a Catch2 test, and update the sole STF consumer (event_types.cuh) to use the new location directly. Made-with: Cursor
|
/ok to test cb65d4f |
😬 CI Workflow Results🟥 Finished in 44m 15s: Pass: 12%/48 | Total: 21h 49m | Max: 44m 09s | Hits: 87%/1282See results here. |
Made-with: Cursor
| return ::cuda::experimental::unstable_unique(__first, __last, [](const auto& __a, const auto& __b) { | ||
| return __a == __b; | ||
| }); |
There was a problem hiding this comment.
Prefer ::cuda::std::equal_to<>{} here I think.
| return __last; | ||
| } | ||
|
|
||
| ++__first; |
There was a problem hiding this comment.
Can be moved into the first clause of the for-loop
There was a problem hiding this comment.
Nice, though we lose the comment (which was lost already lol).
| //! | ||
| //! @return Iterator to the new end of the range after duplicates have been removed. | ||
| template <class _Iterator, class _BinaryPredicate> | ||
| _CCCL_HOST_API _Iterator unstable_unique(_Iterator __first, _Iterator __last, _BinaryPredicate __pred) |
There was a problem hiding this comment.
It seems like this algorithm requires random access iterators. Consider static_assert()-ing (and documenting) that.
Note ++ and -- makes for bidirectional iterators, and I believe < means it needs random-access. If you can make the outer if condition if (; __first != __last; ++__first) and hide the asserts behind
if constexpr (/* is_random_access_iterator */)then you could at least relax to bidirectional.
There was a problem hiding this comment.
Actually it should work with bidir. Though improving the algo wasn't the primary purpose of this PR, we appreciate the review and take the opportunity to polish the code a lil.
I'll add specific suggestions to make the algo work with bidirectional iterators.
|
|
||
| TEST_CASE("unstable_unique empty range", "[utility]") | ||
| { | ||
| std::vector<int> v; |
There was a problem hiding this comment.
Consider adding some tests with other iterator categories. For example std::list or std::set for bidirectional iterators.
| ++__first; | ||
| bool __first_is_known_duplicate = false; | ||
|
|
||
| for (; __first < __last; ++__first) |
There was a problem hiding this comment.
| for (; __first < __last; ++__first) | |
| for (; __first != __last; ++__first) |
to make it work with bidir
| continue; | ||
| } | ||
| } | ||
| _CCCL_ASSERT(__first < __last, "unstable_unique: iterator out of range"); |
There was a problem hiding this comment.
| _CCCL_ASSERT(__first < __last, "unstable_unique: iterator out of range"); | |
| _CCCL_ASSERT(__first != __last, "unstable_unique: iterator out of range"); |
also for bidir
| { | ||
| return __first; | ||
| } | ||
| _CCCL_ASSERT(__first < __last, "unstable_unique: iterator out of range"); |
There was a problem hiding this comment.
| _CCCL_ASSERT(__first < __last, "unstable_unique: iterator out of range"); | |
| _CCCL_ASSERT(__first != __last, "unstable_unique: iterator out of range"); |
| return ::cuda::experimental::unstable_unique(__first, __last, [](const auto& __a, const auto& __b) { | ||
| return __a == __b; | ||
| }); |
There was a problem hiding this comment.
| return ::cuda::experimental::unstable_unique(__first, __last, [](const auto& __a, const auto& __b) { | |
| return __a == __b; | |
| }); | |
| return ::cuda::experimental::unstable_unique(__first, __last, ::cuda::std::equal_to<>{}); |
nice, thx @Jacobfaib
3 participants
unstable_unique is a generic algorithm with no STF dependency. Move it to cuda::experimental namespace under __utility/, replace the STF UNITTEST with a Catch2 test, and update the sole STF consumer (event_types.cuh) to use the new location directly.
Description
closes
Checklist