Consolidate random functions into Random class #1109
Conversation
- remove functions end with vector/matrix - remove functions that take limit argument - improve documentation - add more unit tests
Codecov Report
@@ Coverage Diff @@
## release-6.7 #1109 +/- ##
==============================================
Coverage ? 56.47%
==============================================
Files ? 335
Lines ? 24934
Branches ? 0
==============================================
Hits ? 14081
Misses ? 10853
Partials ? 0
|
| namespace math { | ||
|
|
||
| //============================================================================== | ||
| Random::GeneratorType& Random::getRandGenerator() |
There was a problem hiding this comment.
Why is this getRandGenerator() instead of just getGenerator()? It is already on a Random class and it doesn't seem like there is another generator in this context to worry about 😅
| namespace dart { | ||
| namespace math { | ||
|
|
||
| class Random final |
There was a problem hiding this comment.
If this is a final class which consists entirely of static methods, would it make more sense to make this a normal class and then have a singleton available here for simpler use cases?
That seems like it would be more flexible for situations where someone might want to use this helpful class but also track multiple RNGs with different seeds, where they can instantiate two of the class, while the singleton can be available for cases where that is not necessary.
There was a problem hiding this comment.
This sounds good to me. In any case, I would like to keep the API simple. We could introduce wrapper functions something like this:
dart::math::uniform(min, max) {
return Random::getInstance()::uniform(min, max);
}Let me create an issue for this to track.
This implementation is inspired by
ign-math/Randwith some extensions forEigenobjects.The old-style random functions are deprecated.
The list of functions is not complete yet. Missing functions will be added later such as generating vector/matrix from normal distribution and multivariate normal distribution.
This is a prequel of #1106.
Before creating a pull request
clang-formatBefore merging a pull request
CHANGELOG.md