Coot 10 Change arma::rand n, u, i functions to Rand #3617
Conversation
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
| * | ||
| * @code | ||
| * // 100-point 5-dimensional random dataset. | ||
| * arma::mat data = arma::randu<arma::mat>(5, 100); |
There was a problem hiding this comment.
We shouldn't change the examples.
src/mlpack/core/util/rand.hpp
Outdated
| * the 3-clause BSD license along with ensmallen. If not, see | ||
| * http://www.opensource.org/licenses/BSD-3-Clause for more information. | ||
| */ | ||
| #ifndef MLPACK_CORE_UTIL_RAND_HPP |
There was a problem hiding this comment.
There is already a lot of random number support in math/ (I just documented it). Although these functions are internal, and we shouldn't document them externally, it would still be a good idea to see if we can make them match our existing RNG functionalities a bit better.
There was a problem hiding this comment.
maybe mix this file with that one?
|
Actually, it makes sense to use |
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
|
This is ready for review, I am not adding anything else |
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
|
@rcurtin this is ready finally for a review |
rcurtin
left a comment
rcurtin
left a comment
There was a problem hiding this comment.
Looks good---the changes are much simpler with using instead of ADL. 👍
| DataType sample = arma::randu<DataType> | ||
| (probability.n_rows, probability.n_cols); | ||
| DataType sample; | ||
| sample.randu(probability.n_rows, probability.n_cols); |
There was a problem hiding this comment.
We could go back to the standalone randu now, but, totally up to you---I think it's fine either way.
There was a problem hiding this comment.
To be honest, I would keep it this way, and I think we should do it this way going forward.
| size_t upperBound = full ? capacity : position; | ||
| arma::uvec sampledIndices = arma::randi<arma::uvec>( | ||
| arma::uvec sampledIndices = randi<arma::uvec>( | ||
| batchSize, arma::distr_param(0, upperBound - 1)); |
There was a problem hiding this comment.
This is a comment for later, but I'm interested what will happen here when we remove the arma:: from distr_param. If we have Bandicoot available, then both the arma::distr_param and coot::distr_param classes will be available, and the compiler will have to correctly select arma::distr_param. I think that this will work fine when we do it, but there is a possibility that there could be a problem we need to work around.
Anyway, nothing to do about that now, just a comment.
There was a problem hiding this comment.
I know, when I had ADL I was planning to use an additional template parameter DistrParam. However, there is a high chance that we will not needed for now.
![mlpack-bot[bot]](https://avatars.githubusercontent.com/in/23934?s=60&v=4){kind=small}
2 participants
No description provided.