Repository navigation
feat(rng): add new RNGs and sparse random matrix functions (fixes ##359 - #1450
Conversation
feafd66 to
90532d6
Compare
90532d6 to
9c2888f
Compare
9c2888f to
2f21304
Compare
2f21304 to
06b1c43
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces new random number generators and sparse random matrix functionality to enhance the random number generation capabilities. The main purpose is to expand RNG options and provide compatibility with MATLAB-style sparse random matrix functions.
- Added four new RNG engines: philox, simdTwister, threefry, and combRecursive
- Implemented sparse random matrix functions
sprand(uniform) andsprandn(normal distribution) - Updated existing RNG tests to work with the new generators and removed dependency on Boost random libraries
Reviewed Changes
Copilot reviewed 158 out of 158 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/types/src/cpp/*.cpp | Replace std::nan("NaN") with std::nan("") for consistent NaN handling |
| modules/string/src/cpp/*.cpp | Same NaN parameter standardization across string modules |
| modules/statistics/src/cpp/*.cpp | NaN parameter updates for statistics functions |
| modules/spreadsheet/src/cpp/*.cpp | Consistent NaN handling in spreadsheet operations |
| modules/special_functions/src/cpp/*.cpp | NaN standardization in interpolation functions |
| modules/sparse/functions/*.m | New MATLAB functions for sparse random matrix generation |
| modules/sparse/help/en_US/xml/*.xml | Documentation for new sparse matrix functions |
| modules/sparse/tests/*.m | Test files for sparse random matrix functionality |
| modules/random/src/cpp/*.cpp | Major refactoring to remove Boost dependencies and add new RNG engines |
| modules/random/tests/*.m | Updated and new test files for all RNG engines |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
b2f9381 to
5048242
Compare
| double val1 = std::sqrt(-2.0 * std::log(u1)) * std::cos(2.0 * M_PI * u2); | ||
| double val2 = std::sqrt(-2.0 * std::log(u1)) * std::sin(2.0 * M_PI * u2); | ||
| ar[i] = val1; | ||
| ++i; |
Check notice
Code scanning / CodeQL
For loop variable changed in body
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
To fix this issue, we should replace the for loop that uses both j and i as variables (and increments i within the body), with an explicit while loop over i, managing j manually. This gives clear control and eliminates the confusion around loop counters. More specifically, in lines 99-124 of modules/random/src/cpp/RandomPhilox.hpp, the inner for loop should be converted to a while loop.
- Define
j = 0before the loop and usewhile (j < 4 && i < nbElements)for iteration. - Replace the current usage of
for (int j = 0; ...)with an explicitjvariable. - At each pass, increment
jandionly as needed, preserving the current correctness. - All other logic inside the loop remains unchanged.
- No new methods or imports are required.
| @@ -96,10 +96,13 @@ | ||
| while (i < nbElements) { | ||
| generateBuffer(); | ||
| counter[2] += 2; | ||
| for (int j = 0; j < 4 && i < nbElements; ++j, ++i) { | ||
| int j = 0; | ||
| while (j < 4 && i < nbElements) { | ||
| uint32_t nextUint32 = buffer[j]; | ||
| if (_type == RNG_DISTRIBUTION_UNIFORM_REAL) { | ||
| ar[i] = static_cast<double>(nextUint32) / 4294967296.0; | ||
| ++j; | ||
| ++i; | ||
| } else if (_type == RNG_DISTRIBUTION_NORMAL) { | ||
| if (i + 1 < nbElements) { | ||
| uint32_t u1_int = nextUint32; | ||
| @@ -112,14 +112,19 @@ | ||
| ++i; | ||
| if (i < nbElements) { | ||
| ar[i] = val2; | ||
| ++i; | ||
| } | ||
| ++j; | ||
| j += 2; | ||
| } else { | ||
| double u1 = (static_cast<double>(nextUint32) + 0.5) / 4294967296.0; | ||
| ar[i] = std::sqrt(-2.0 * std::log(u1)) * std::cos(2.0 * M_PI * 0.5); | ||
| ++j; | ||
| ++i; | ||
| } | ||
| } else if (_type == RNG_DISTRIBUTION_UNIFORM_INT) { | ||
| ar[i] = static_cast<double>(_min + (nextUint32 % (_max - _min + 1))); | ||
| ++j; | ||
| ++i; | ||
| } | ||
| } | ||
| } |
| if (i < nbElements) { | ||
| ar[i] = val2; | ||
| } | ||
| ++j; |
Check notice
Code scanning / CodeQL
For loop variable changed in body
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
To fix this problem, we should replace the for loop that modifies the j loop variable in its body with a while loop, which better matches the logic when the loop counter can be changed irregularly. Specifically, in getValuesAsDouble, we must alter lines 99–124: Instead of for (int j = 0; j < 4 && i < nbElements; ++j, ++i), use a while loop with explicit control over both indices j and i. The loop body remains the same, but wherever j or i are incremented elsewhere in the block, care must be taken to not duplicate increments on exiting the loop body.
No changes are needed outside the lines indicated, and the rest of the behavior will remain. No new includes or method definitions are required.
| @@ -96,10 +96,13 @@ | ||
| while (i < nbElements) { | ||
| generateBuffer(); | ||
| counter[2] += 2; | ||
| for (int j = 0; j < 4 && i < nbElements; ++j, ++i) { | ||
| int j = 0; | ||
| while (j < 4 && i < nbElements) { | ||
| uint32_t nextUint32 = buffer[j]; | ||
| if (_type == RNG_DISTRIBUTION_UNIFORM_REAL) { | ||
| ar[i] = static_cast<double>(nextUint32) / 4294967296.0; | ||
| ++i; | ||
| ++j; | ||
| } else if (_type == RNG_DISTRIBUTION_NORMAL) { | ||
| if (i + 1 < nbElements) { | ||
| uint32_t u1_int = nextUint32; | ||
| @@ -113,13 +113,18 @@ | ||
| if (i < nbElements) { | ||
| ar[i] = val2; | ||
| } | ||
| ++j; | ||
| ++i; | ||
| j += 2; | ||
| } else { | ||
| double u1 = (static_cast<double>(nextUint32) + 0.5) / 4294967296.0; | ||
| ar[i] = std::sqrt(-2.0 * std::log(u1)) * std::cos(2.0 * M_PI * 0.5); | ||
| ++i; | ||
| ++j; | ||
| } | ||
| } else if (_type == RNG_DISTRIBUTION_UNIFORM_INT) { | ||
| ar[i] = static_cast<double>(_min + (nextUint32 % (_max - _min + 1))); | ||
| ++i; | ||
| ++j; | ||
| } | ||
| } | ||
| } |
| float u2 = (static_cast<float>(u2_int) + 0.5f) / 4294967296.0f; | ||
| ar[i] = static_cast<single>( | ||
| std::sqrt(-2.0f * std::log(u1)) * std::cos(2.0f * M_PI * u2)); | ||
| ++i; |
Check notice
Code scanning / CodeQL
For loop variable changed in body
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
To resolve the issue, refactor the inner for loop on lines 136–162 into a while loop so that both loop variables i and j can be controlled explicitly and incremented as needed within the loop body. This eliminates the ambiguity and maintains the correct logic regardless of how many times i is incremented in a single iteration. The outer structure and function should remain unchanged, and the inner logic should be ported to a while loop that handles increments to i and j explicitly.
Required changes:
- Replace the
forloop with awhileloop, with the same exit conditions:j < 4 && i < nbElements. - Manually increment
jandias needed, matching the original branching logic. - No new imports/definitions are needed.
| @@ -133,10 +133,13 @@ | ||
| while (i < nbElements) { | ||
| generateBuffer(); | ||
| counter[2] += 2; | ||
| for (int j = 0; j < 4 && i < nbElements; ++j, ++i) { | ||
| int j = 0; | ||
| while (j < 4 && i < nbElements) { | ||
| uint32_t nextUint32 = buffer[j]; | ||
| if (_type == RNG_DISTRIBUTION_UNIFORM_REAL) { | ||
| ar[i] = static_cast<float>(nextUint32) / 4294967296.0f; | ||
| ++j; | ||
| ++i; | ||
| } else if (_type == RNG_DISTRIBUTION_NORMAL) { | ||
| if (i + 1 < nbElements) { | ||
| uint32_t u1_int = nextUint32; | ||
| @@ -149,15 +149,20 @@ | ||
| if (i < nbElements) { | ||
| ar[i] = static_cast<single>( | ||
| std::sqrt(-2.0f * std::log(u1)) * std::sin(2.0f * M_PI * u2)); | ||
| ++i; | ||
| } | ||
| ++j; | ||
| j += 2; // We used j and j+1 elements from buffer | ||
| } else { | ||
| float u1 = (static_cast<float>(nextUint32) + 0.5f) / 4294967296.0f; | ||
| ar[i] = static_cast<single>( | ||
| std::sqrt(-2.0f * std::log(u1)) * std::cos(2.0f * M_PI * 0.5f)); | ||
| ++j; | ||
| ++i; | ||
| } | ||
| } else if (_type == RNG_DISTRIBUTION_UNIFORM_INT) { | ||
| ar[i] = static_cast<single>(_min + (nextUint32 % (_max - _min + 1))); | ||
| ++j; | ||
| ++i; | ||
| } | ||
| } | ||
| } |
| ar[i] = static_cast<single>( | ||
| std::sqrt(-2.0f * std::log(u1)) * std::sin(2.0f * M_PI * u2)); | ||
| } | ||
| ++j; |
Check notice
Code scanning / CodeQL
For loop variable changed in body
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
The correct fix is to replace the inner for loop (lines 136–162) with a while loop over j, since j is being manipulated not only in the loop header (with ++j), but also inside the body (specifically, inside the normal distribution branch where it may be incremented by 2 on some iterations). Replace the for loop with logic that handles j incrementing exactly as intended, and remove confusion. Implement all required logic within the while loop, including the manual increment(s), and ensure the code remains functionally unchanged. The rest of the function should remain as is, and no new imports are needed.
| @@ -133,10 +133,13 @@ | ||
| while (i < nbElements) { | ||
| generateBuffer(); | ||
| counter[2] += 2; | ||
| for (int j = 0; j < 4 && i < nbElements; ++j, ++i) { | ||
| int j = 0; | ||
| while (j < 4 && i < nbElements) { | ||
| uint32_t nextUint32 = buffer[j]; | ||
| if (_type == RNG_DISTRIBUTION_UNIFORM_REAL) { | ||
| ar[i] = static_cast<float>(nextUint32) / 4294967296.0f; | ||
| ++j; | ||
| ++i; | ||
| } else if (_type == RNG_DISTRIBUTION_NORMAL) { | ||
| if (i + 1 < nbElements) { | ||
| uint32_t u1_int = nextUint32; | ||
| @@ -150,14 +150,18 @@ | ||
| ar[i] = static_cast<single>( | ||
| std::sqrt(-2.0f * std::log(u1)) * std::sin(2.0f * M_PI * u2)); | ||
| } | ||
| ++j; | ||
| j += 2; | ||
| } else { | ||
| float u1 = (static_cast<float>(nextUint32) + 0.5f) / 4294967296.0f; | ||
| ar[i] = static_cast<single>( | ||
| std::sqrt(-2.0f * std::log(u1)) * std::cos(2.0f * M_PI * 0.5f)); | ||
| ++j; | ||
| ++i; | ||
| } | ||
| } else if (_type == RNG_DISTRIBUTION_UNIFORM_INT) { | ||
| ar[i] = static_cast<single>(_min + (nextUint32 % (_max - _min + 1))); | ||
| ++j; | ||
| ++i; | ||
| } | ||
| } | ||
| } |
- Introduced new random number generators: philox, simdTwister, threefry, combRecursive - Added functions `sprand` (uniform) and `sprandn` (normal) for sparse random matrices - Updated tests for compatibility with new RNGs - Added XML docs for `sprand` and `sprandn`
5048242 to
1cc54c0
Compare
randi,sprand(uniform) andsprandn(normal) for sparse random matricessprandandsprandn