Add O(sample_size) sampling algorithm under cuda/ - #11000
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
Jacobfaib
changed the title
O(sample_size) sampling algorithm under cuda/
|
do you have in mind specific applications for the sampling algorithm? |
|
The original application was #10939, but for a few reasons I ended up being able to trade off a slightly subpar sampling in exchange for much better perf there. The algorithm in this PR is a fully correct sampling scheme that should get better perf than regular |
This comment has been minimized.
This comment has been minimized.
| //! Unlike `cuda::std::sample`, which reads every population element, this algorithm reads exactly | ||
| //! the `min(__n, __last - __first)` selected elements and draws `O(__n)` random numbers. It requires |
There was a problem hiding this comment.
suggestion: This feels like sufficiently different behavior that it warrants a different name from cuda::sample. Something like cuda::minimal_sample?
There was a problem hiding this comment.
It does the same thing as std::sample(), as in any place you'd want to call std::sample(), you'd also want to call this function. I'm actually not sure why this algorithm wasn't standardized, it is an evolution of Knuths reservoir sampling which is what std::sample() does.
There was a problem hiding this comment.
¯\_(ツ)_/¯ put it up for a vote on Slack and see what the language lawyers have to say.
|
@RAMitchell is our resident random sampling expert and would be a good person to have review this. |
|
Benchmark results: As shown, as population size increases, |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded ChangesCUDA sampling
Suggested reviewers: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libcudacxx/include/cuda/__algorithm/sample.h (1)
315-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
__n >= 0is still compiled when_Distanceis unsigned, which produces a tautological-comparison diagnostic on some compilers. Use the prescribed form.Proposed assert form
- _CCCL_ASSERT(!::cuda::std::is_signed_v<_Distance> || __n >= 0, "N must be a positive number."); + _CCCL_ASSERT(!(::cuda::std::is_unsigned_v<_Distance> ? false : (__n < 0)), "N must be a positive number.");The include would change from
<cuda/std/__type_traits/is_signed.h>to<cuda/std/__type_traits/is_unsigned.h>. As per coding guidelines: "Unsigned variables, or variables that may become unsigned after template instantiation, must not be compared against negative values directly; usecuda::std::is_unsigned_v<T> ? false : (var < 0)instead."Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5dac5f9b-2fa5-4a2a-b38c-985feb2a102c
📒 Files selected for processing (4)
libcudacxx/benchmarks/bench/sample/basic.culibcudacxx/include/cuda/__algorithm/sample.hlibcudacxx/include/cuda/algorithmlibcudacxx/test/libcudacxx/cuda/algorithm/sample.cu
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libcudacxx/benchmarks/bench/sample/basic.cu (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Declare both
endvariables asconst auto* const.The pointer values are not reassigned. This also applies to the
cuda::std::samplekernel.
libcudacxx/benchmarks/bench/sample/basic.cu#L30-L30: declareendas a const pointer.libcudacxx/benchmarks/bench/sample/basic.cu#L39-L39: declareendas a const pointer.As per coding guidelines, “All variables that are not modified must be declared
const, including cast results, function return values, and loop-invariant computations.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 86905dcb-54ef-46dd-a4bc-95f77d1bae30
📒 Files selected for processing (1)
libcudacxx/benchmarks/bench/sample/basic.cu
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
This comment has been minimized.
This comment has been minimized.
RAMitchell
left a comment
RAMitchell
left a comment
There was a problem hiding this comment.
I've never implemented this algorithm so I will assume its a correct textbook implementation.
I believe this is also a valid implementation. We lack cuda::std::sort though I think? The shuffle iterator is also pretty heavyweight. Maybe useful for the sake of comparison.
N = last - first
k = min(n, N)
shuffled =
cuda::shuffle_iterator{cuda::random_bijection{N, rng}}
cuda::std::copy_n(shuffled, k, scratch)
sort(scratch, scratch + k)
selected =
cuda::permutation_iterator{first, scratch}
return cuda::std::copy_n(selected, k, output)
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 55e90984-bee9-4a47-9bf3-25cf3c17cca3
📒 Files selected for processing (1)
libcudacxx/test/libcudacxx/cuda/algorithm/sample.cu
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
4bba3fa to
8de49c9
Compare
This comment has been minimized.
This comment has been minimized.
RAMitchell
left a comment
RAMitchell
left a comment
There was a problem hiding this comment.
Tests LGTM, just check my one comment. I would be interested to see a benchmark against the simpler shuffle iterator version above, but non-essential.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| auto __s = _Distance{0}; | ||
| auto __quot = __top / __N_real; | ||
|
|
||
| while (__quot > __v) |
There was a problem hiding this comment.
I am a bit terrified by the floating point math here, but I am not sure what the appropriate translation into integer domain would be
🥳 CI Workflow Results🟩 Finished in 1h 41m: Pass: 100%/183 | Total: 2d 07h | Max: 1h 18m | Hits: 98%/395769See results here. AI failure analysis1. cugraph rmm::device_scalar zero initialization references a deleted rvalue constructor · 1 jobExplanation: All compiler diagnostics have the same overload-resolution mechanism: cugraph passes temporary zero values such as T{0} to rmm::device_scalar, selecting RMM 26.10's deleted const value_type&& constructor. The PR changes libcudacxx sampling and copy/move headers, so the logs alone do not establish whether it exposed this incompatibility or the checked-out RAPIDS main revisions are independently mismatched; comparison against the base CCCL revision is needed. Evidence: Copy this prompt into a coding agentJobs: |
6 participants
Description
std::sample()isO(population_size), notO(sample_size)including for random access iterators.Adds a true
O(sample_size)random sampling implementation based on a hybrid of Vitters sampling algorithm A and D (https://www.ittc.ku.edu/~jsv/Papers/Vit87.RandomSampling.pdf).I do not pretend to fully know how the algorithm works, but the key idea seems to be that instead of performing an elementwise test of whether it should be kept or rejected, the algorithm determines how many elements should be skipped before choosing another element. This ensures the algorithm is indeed
O(sample_size)because the size of the skip is entirely dependent onsample_size.Checklist