Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use Armadillo RNG support #6

Closed
rcurtin opened this issue Oct 6, 2018 · 8 comments
Closed

use Armadillo RNG support #6

rcurtin opened this issue Oct 6, 2018 · 8 comments

Comments

@rcurtin
Copy link
Member

rcurtin commented Oct 6, 2018

There are a couple instances where mlpack's Random() or RandInt() functions are used:

./parallel_sgd/parallel_sgd_impl.hpp:          mlpack::math::randGen);
./sa/sa_impl.hpp:  const double unif = 2.0 * math::Random() - 1.0;
./sa/sa_impl.hpp:  const double xi = math::Random();
./scd/descent_policies/random_descent.hpp:    return mlpack::math::RandInt(function.NumFeatures());

I think this shows up a lot in the tests too. Instead, we can use Armadillo's RNG here; so mlpack::math::randGen can (probably) be replaced by arma::arma_rng_cxx11_instance or something similar, and we can use that to generate random numbers.

However there is also the Armadillo configuration parameter ARMA_USE_EXTERN_CXX11_RNG; but, looking through it quickly, I don't see anything different for us to do whether or not someone using ensmallen has written #define ARMA_USE_EXTERN_CXX11_RNG.

@conradsnicta
Copy link
Contributor

conradsnicta commented Oct 7, 2018

Armadillo has an internal class called arma_rng_cxx11, but it's not part of the public API.

The ARMA_USE_EXTERN_CXX11_RNG define is set by the armadillo cmake installer to indicate that the arma::arma_rng_cxx11_instance is available in the armadillo wrapper library. The define not meant to be used outside of that specific use case. Furthermore, it's only valid on Linux systems with gcc 4.8.3+. It currently doesn't work with macOS or windoze. (it should be trivial to make it work with macOS, now that Apple has proper support for thethread_local storage class in C++11).

Perhaps it would be sufficient to simply use the public randi(), randu(), randn() and randg() functions ?

In armadillo version 8.400+ all of the above functions were extended to output single scalars.
randg() was added in version 4.650. randi() was added in version 3.930.
randu() and randn() have been available "since forever".

If armadillo 8.400 is "too recent", it's still possible to use the above functions to generate scalars. Simply use as_scalar() on a random vector that has only one dimension.

As a sidenote, armadillo 8.400 is available in Ubuntu 18.04.

@zoq
Copy link
Member

zoq commented Oct 7, 2018

In case of:

std::shuffle(visitationOrder.begin(), visitationOrder.end(), mlpack::math::randGen);

it should be reasonable to use:

visitationOrder = arma::shuffel(visitationOrder);

any thoughts?

@zoq
Copy link
Member

zoq commented Oct 7, 2018

For the other cases I agree, it would be simple to use: arma::as_scalar(randi(...)).

@rcurtin
Copy link
Member Author

rcurtin commented Oct 7, 2018

Agreed on both counts---I wonder why we didn't just use arma::shuffle(visitationOrder) originally?

@zoq
Copy link
Member

zoq commented Oct 8, 2018

I guess the benefit is that std::shuffel works inplace; should be neglectable here.

@zoq
Copy link
Member

zoq commented Oct 8, 2018

Thanks for the clarification, should have checked first. 05a8743 implements the modifications.

@rcurtin
Copy link
Member Author

rcurtin commented Oct 18, 2018

I think this one is fully handled now---I see no use of any random object or anything other than Armadillo's existing public random functions.

@rcurtin rcurtin closed this as completed Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants