Enabling updated rng in oneDAL algos #3153
Conversation
|
/intelci: run |
|
|
||
| const SizeType range = b - a; | ||
| Type * buffer = (Type *)daal_malloc(sizeof(Type) * range); | ||
| if (!buffer) return -2; // Allocation failure |
There was a problem hiding this comment.
| if (!buffer) return -2; // Allocation failure | |
| if (!buffer) return services::ErrorMemoryAllocationFailed; // Allocation failure |
| Type * buffer = (Type *)daal_malloc(sizeof(Type) * n); | ||
| if (n > (b - a)) | ||
| { | ||
| return -1; // Error: n is greater than the range |
There was a problem hiding this comment.
Please use error indices from "error_indexes.h" instead of integer constants. For example:
| return -1; // Error: n is greater than the range | |
| return services::ErrorIncorrectSizeOfArray; // Error: n is greater than the range |
You can also define new index in case there is no index that suites your needs.
| } | ||
| r[i] = value; | ||
| int j; | ||
| errorcode = uniform(1, &j, state, a + i, b, method); |
There was a problem hiding this comment.
Can you add a reference to the method that is used here?
| engine_.skip_ahead_cpu(count); | ||
| auto rand_event = generate_rng(distr, engine_, count, rand_ptr, { fill_event }); | ||
|
|
||
| // // Perform Fisher-Yates shuffle for first 'count' elements |
There was a problem hiding this comment.
| // // Perform Fisher-Yates shuffle for first 'count' elements | |
| // Perform Fisher-Yates shuffle for first 'count' elements |
| if (count > n) { | ||
| throw std::invalid_argument("Sample size exceeds population size"); | ||
| } |
There was a problem hiding this comment.
A check that count > 0 is also needed here.
| cgh.depends_on(fill_event); | ||
| cgh.single_task([=]() { | ||
| for (std::int64_t idx = 0; idx < count; ++idx) { | ||
| // // Generate random index between idx and n-1 (inclusive) |
There was a problem hiding this comment.
| // // Generate random index between idx and n-1 (inclusive) | |
| // Generate random index between idx and n-1 (inclusive) |
| auto rand_array = array<Type>::empty(queue, count); | ||
| Type* rand_ptr = rand_array.get_mutable_data(); | ||
|
|
||
| oneapi::mkl::rng::uniform<Type> distr(0, n); |
There was a problem hiding this comment.
Please mention somewhere in the description that this implementation only works with std::int32_t and std::uint32_t as Type, because oneMKL supports only those 2 types for discrete uniform distribution.
Or use oneapi::mkl::rng::uniform<std::int32_t> here like it is done in partial_fisher_yates_shuffle. In this case less restrictions on Type are required.
| Type* idx_ptr = idx_array.get_mutable_data(); | ||
|
|
||
| // Generate random indices for shuffle | ||
| oneapi::mkl::rng::uniform<Type> distr(0, count); |
There was a problem hiding this comment.
Same note about the Type as in uniform_without_replacement needs to be added here.
| this->check_unique_results(arr_gpu); | ||
| this->check_unique_results(arr_host); |
There was a problem hiding this comment.
Should arr_host and arr_gpu be equal?
| pr::device_engine engine_gpu = ::oneapi::dal::backend::primitives::device_engine( | ||
| queue, | ||
| 777, | ||
| ::oneapi::dal::backend::primitives::engine_type_internal::philox4x32x10); |
There was a problem hiding this comment.
Should RNG also be an input parameter in kselect_by_rows_quick with philox4x32x10 as a default value?
2 participants
Description
Add a comprehensive description of proposed changes
List associated issue number(s) if exist(s): #6 (for example)
Documentation PR (if needed): #1340 (for example)
Benchmarks PR (if needed): IntelPython/scikit-learn_bench#155 (for example)
PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance