-
Notifications
You must be signed in to change notification settings - Fork 761
Shuffle #1085
Shuffle #1085
Conversation
Yes, I'd suggest |
8ed52fa to
a63078b
Compare
griwes
left a comment
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 pretty good, I only have a few comments. I've noticed some typos in the doxygen, I'll try to do another pass through that soon.
Looking at the individual commits, I think we should squash this down to a single one before we merge.
alliepiper
left a comment
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 will cause issues with C++11 compilers, but once that's cleared up this patch LGTM.
|
Thank for the quick updates! I'll take it from here and starting validating this against our internal CI. Just a heads up, I'll need to squash this into a single commit before merging. I'll push an update that does that soon. |
|
Local build is clean, tests pass. CI started under shelve 28225884. |
08d40f3 to
fea984d
Compare
Resolve linux compilation, add benchmark Add shuffle_copy, tidy up and comment doxygen Address review comments Silence warnings Guard c++11
Implementation of thrust::shuffle following std::shuffle API.
Todo:
I'm wondering if I should add another version of this function specifying an output iterator to avoid the copy?
Benchmark results for shuffle (on DGX1 V100):