Vectorized unbounded continuous random number generators (Issue 45) #622

bbbales2 · 2017-09-17T18:17:02Z

Submission Checklist

Run unit tests: ./runTests.py test/unit
Run cpplint: make cpplint
Declare copyright holder and open-source license: see below

Summary:

This implements vectorized versions of all the unbounded continuous random number generators (with tests! Hooray!)

Details

I had to make some changes to test/unit/math/prim/mat/prob/vector_rng_test_helper.hpp to allow for anywhere from one to three parameters to be tested.

I also had to update the way the Skew normal random numbers were getting generated to avoid infs (this is part of Issue #527)

I also fixed some indents in test/unit/math/prim/scal/prob/util.hpp and added an extra Google test check there (that will throw if the code tries to do something that leads to segfaults in the tests).

@seantalts Hope you were looking forward to checking some repetitive code

How to Verify:

./runTests.py test/unit/math/prim/mat/prob/normal_test.cpp
./runTests.py test/unit/math/prim/mat/prob/exp_mod_normal_test.cpp
./runTests.py test/unit/math/prim/mat/prob/skew_normal_test.cpp
./runTests.py test/unit/math/prim/mat/prob/student_t_test.cpp
./runTests.py test/unit/math/prim/mat/prob/cauchy_test.cpp
./runTests.py test/unit/math/prim/mat/prob/double_exponential_test.cpp
./runTests.py test/unit/math/prim/mat/prob/logistic_test.cpp
./runTests.py test/unit/math/prim/mat/prob/gumbel_test.cpp

And the scalar tests (though I didn't change these)

./runTests.py test/unit/math/prim/scal/prob/normal_test.cpp
./runTests.py test/unit/math/prim/scal/prob/exp_mod_normal_test.cpp
./runTests.py test/unit/math/prim/scal/prob/skew_normal_test.cpp
./runTests.py test/unit/math/prim/scal/prob/student_t_test.cpp
./runTests.py test/unit/math/prim/scal/prob/cauchy_test.cpp
./runTests.py test/unit/math/prim/scal/prob/double_exponential_test.cpp
./runTests.py test/unit/math/prim/scal/prob/logistic_test.cpp
./runTests.py test/unit/math/prim/scal/prob/gumbel_test.cpp

Side Effects:

Hopefully none

Documentation:

It's all inline

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): University of California, Santa Barbara

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

…3 parameter rngs (Issue 45) Vectorized all unbounded continuous random number generators and added tests (Issue 45) Changed how skew_normal random numbers are generated (following https://arxiv.org/pdf/0911.2342.pdf) (Issue 527)

bbbales2 · 2017-09-17T23:18:25Z

Poor ol' Travis. Package manager failed for some reason it seems.

bob-carpenter · 2017-09-18T19:11:22Z

This is failing in unit testing because a new signed comparison warning cropped up in the test code. Easy fix to change an int to a size_t.

bbbales2 · 2017-09-18T19:24:10Z

Bah, I use clang++ for the nice errors, but I should probably just switch to g++ cause it seems to be more careful about these things. Tnx for the heads up.

bbbales2 · 2017-09-21T15:09:37Z

I think this is ready to review. Jenkins failed on some upstream tests for some mystery reason (edit: but the Math tests passed):

Build step 'Execute shell' marked build as failure
ERROR: Step ‘Scan for compiler warnings’ failed: no workspace for Math Pull Request - Upstream - Stan #450
ERROR: Step ‘Publish JUnit test result report’ failed: no workspace for Math Pull Request - Upstream - Stan #450
ERROR: gelman-group-linux is offline; cannot locate JDK8u66

Could easily ask for a retest, but that'll burn another pile of hours on Jenkins for nothing. (no way this gets through without revisions anyway)

seantalts · 2017-09-21T16:28:51Z

Sorry, the linux node has been tricksy so typically we'll just retest when this happens. I'm working on getting it to be more friendly. Bob, are you reviewing this or should someone else (me?)?

bbbales2 · 2017-09-21T16:39:03Z

tricksy

I just made it through the Hobbit again. Fantastic book. I'm totally into a Golem rebranding of Jenkins.

Maybe if we wanted a rebuild we'd haves to answers one of precious's riddles?

bbbales2 · 2017-09-21T17:13:04Z

Jenkins retest this please

bbbales2 · 2017-09-24T19:53:47Z

Jenkins retest this please

…torized_rngs_unbounded_continuous

bbbales2 · 2017-09-27T17:33:59Z

Nice! Tests passed with the gcc-4.9 patch in place. This is ready for review.

bob-carpenter

Thanks. Overall, this looks really good but I'm asking for a bunch of code cleanup. See inline comments.

I marked which changes were optional but do ask if it's not clear.

bob-carpenter · 2017-09-27T17:31:52Z

stan/math/prim/scal/prob/cauchy_rng.hpp

-               double sigma,
+    template <typename T_loc, typename T_scale, class RNG>
+    inline typename VectorBuilder<true, double, T_loc, T_scale>::type
+    cauchy_rng(const T_loc &mu,


The cpplint thing isn't very strict. We've been putting& after type, not before variable, as in T_loc& mu. I'd rather keep our code base consistent. Also, put as many arguments on a line as will fit and generally try to conserve vertical space as much as possible.

bob-carpenter · 2017-09-27T17:38:34Z

stan/math/prim/scal/prob/double_exponential_rng.hpp

+        variate_generator<RNG&, uniform_01<> > rng_unit_01(rng, uniform_01<>());
+        double a = 0;
+        double laplaceRN = rng_unit_01();
+        if (0.5 - laplaceRN > 0)


In general, you want to declare variables before you use them and only define them once if possible. So this should be just

double a = rng_unit_01() < 0.5 ? 1 : -1;

Then you could just replace a with the above. And you'd want to move the declaration and definition of rng_unit_01 out of the loop for efficiency

Having said all that, I think it'll be easier to just write a function that does bernoulli_rng without all the checks. This is too much logic in this function to generate random bernoulli variates.

bob-carpenter · 2017-09-27T17:42:02Z

stan/math/prim/scal/prob/exp_mod_normal_rng.hpp

+      VectorBuilder<true, double, T_loc, T_scale, T_inv_scale> output(N);
+
+      for (size_t n = 0; n < N; n++) {
+        output[n] = normal_rng(mu_vec[n], sigma_vec[n], rng) +


This isn't going to be too much of an efficiency issue, but generally you don't want to call your public-facing functions internally because they're going to be doing redundant argument checks. It'll wind up creating more objects, too. Overally, though, this is probably OK like this and not worth optimizing now.

bob-carpenter · 2017-09-27T17:43:15Z

stan/math/prim/scal/prob/gumbel_rng.hpp

+      VectorBuilder<true, double, T_loc, T_scale> output(N);
+
+      for (size_t n = 0; n < N; n++) {
+        variate_generator<RNG&, uniform_01<> >


Pull the rng out of the loop for efficiency.

bob-carpenter · 2017-09-27T17:43:37Z

stan/math/prim/scal/prob/logistic_rng.hpp

+      VectorBuilder<true, double, T_loc, T_scale> output(N);
+
+      for (size_t n = 0; n < N; n++) {
+        variate_generator<RNG&, exponential_distribution<> >


rng out of loop. you should be doing this for every rng that doesn't depend on the loop parameters. it saves a lot of object construction and destruction and still uses the same RNG so the result will the be same

bob-carpenter · 2017-09-27T17:48:56Z

test/unit/math/prim/mat/prob/cauchy_test.cpp

+#include <limits>
+#include <vector>
+
+using Eigen::Dynamic;


You don't want to have using statements up at file scope---keep them limited to tests. This is absolutely forbidden in non-test code and we're trying to clean up our tests, too. It's too easy to pollute the namespaces this way.

bob-carpenter · 2017-09-27T17:52:19Z

test/unit/math/prim/mat/prob/cauchy_test.cpp

+std::vector<double> build_quantiles(int N, double mu, double sigma,
+                                    double unused) {
+  std::vector<double> quantiles;
+  int K = boost::math::round(2 * std::pow(N, 0.4));


K should be defined as a double to begin with (round returns double here); then you don't need to cast anything in i / K.

bob-carpenter · 2017-09-27T17:53:20Z

test/unit/math/prim/mat/prob/vector_rng_test_helper.hpp

@@ -21,6 +21,9 @@
 template <typename T_param>
 void assign_parameter_values(T_param& params,
                             const std::vector<double>& values) {
+  if(values.size() == 0)
+    return;
+
  // Static cast the size here because sometimes it is unsigned (if T_param
  // is a std::vector) and sometimes it is signed (when it is an Eigen type)


There's a general way to do this with a metaprogram to pull the index out.

You should only ever cast as a very last resort. Again, this is just test code, so I'm not going to be too picky, but this isn't how we'd do this in actual Stan code.

bob-carpenter · 2017-09-27T17:53:34Z

test/unit/math/prim/mat/prob/vector_rng_test_helper.hpp

@@ -36,6 +39,9 @@ void assign_parameter_values(T_param& params,
 */
 template <>
 void assign_parameter_values(double& param, const std::vector<double>& values) {
+  if(values.size() == 0)


space before parens in conditionals --- this isn't R!

bob-carpenter · 2017-09-27T18:05:45Z

test/unit/math/prim/mat/prob/vector_rng_test_helper.hpp

+  check_dist_throws<RowVectorXd, VectorXd, RowVectorXd>
+    (generate_samples, good_p1, bad_p1, good_p2, bad_p2, good_p3, bad_p3);
+  check_dist_throws<RowVectorXd, RowVectorXd, RowVectorXd>
+    (generate_samples, good_p1, bad_p1, good_p2, bad_p2, good_p3, bad_p3);


Whenever you see this much redundant code, you want to ask yourself if it's really necessary.

My first inclination here would be to wrap up the arguments (generate_samples, good_p1, bad_p1, good_p2, bad_p2, good_p3, bad_p3) into a structure. Then it's just check_dist_throws<T1, T2, T3>(s) where s is the holding structure. That also simplifies the argument declaration for check_dist_throws. I think you should make this change.

I think you also want to be checking int---it can sometimes cause problems you don't see with double with argument ambiguity resolution. I also think you should make this change.

My second inclination would be to use typelists to iterate over all the types. It's a template programming technique worth learning and a little recursion will deal with all the permutations in a way that's probably easier to validate is correct. The problem with these big lists is that it's very hard to check that they're complete. And they're hard to maintain when you add a new type. This change is optional, but I'd encourage you to at least check out the template technique and start looking over your shoulder every time you think about creating long lists like this.

…torized_rngs_unbounded_continuous

…hing) 1. Switched to typelists for generating all combinations of tests 2. Random number generators are now tested by building test rigs (that inherit from an abstract base class) and passing those to test functions 3. Wasn't able to easily add tests for ints This is all Issue 45

bbbales2 · 2017-10-01T22:29:17Z

Hopefully the tests pass. I changed stuff around quite a bit to address the comments.

My first inclination here would be to wrap up the arguments (generate_samples, good_p1, bad_p1, good_p2, bad_p2, good_p3, bad_p3) into a structure.

I changed how _rngs are tested. Now in each test you inherit from an abstract base class and build a special little object that has the right test hooks (see: https://github.com/bbbales2/math/blob/275889c5ba9185ea8cf6ec70b516013e4f63cb6f/test/unit/math/prim/mat/prob/normal_test.cpp). It's not actually a totally correct abstract base class cause the templated member function can't be virtual, but I thought this makes the test code look nice.

My second inclination would be to use typelists to iterate over all the types.

Hopefully done. There were a bunch of options. I went with a recursive templating solution. There was some variadic templating stuff but it was beyond me. It was tricky to get the right combination of functors/templating so that all the pieces fit together, but it's not so bad now.

I think you also want to be checking int

I did not add integer tests (or std::vector<int> tests). I guess it's important and I should do some thinking, but I might need to add extra lists of good/bad integer values. Rounding the good/bad double values won't always work (cause rounding a good value might give you an invalid value).

"vector_rng_test_helper.hpp" is starting to get pretty verbose with all its little helpers and functions in it. Should I be breaking this out into pieces? If so, any suggestions on what needs broken out?

I imagine this stuff is going to keep growing as the rest of the distribution things get added, so maybe it's worth waiting to see if any other big structural changes come along before we go through the process of breaking things out.

bob-carpenter · 2017-10-02T16:45:33Z

On Oct 1, 2017, at 6:29 PM, Ben Bales ***@***.***> wrote: Hopefully the tests pass. I changed stuff around quite a bit to address the comments. My first inclination here would be to wrap up the arguments (generate_samples, good_p1, bad_p1, good_p2, bad_p2, good_p3, bad_p3) into a structure. I changed how _rngs are tested. Now in each test you inherit from an abstract base class and build a special little object that has the right test hooks (see: https://github.com/bbbales2/math/blob/275889c5ba9185ea8cf6ec70b516013e4f63cb6f/test/unit/math/prim/mat/prob/normal_test.cpp). It's not actually a totally correct abstract base class cause the templated member function can't be virtual, but I thought this makes the test code look nice.

I just brought up this same issue w.r.t. the "base class" we want for whole models. Not sure what the right answer is without being able to have virtual template methods.

My second inclination would be to use typelists to iterate over all the types. Hopefully done. There were a bunch of options. I went with a recursive templating solution. There was some variadic templating stuff but it was beyond me. It was tricky to get the right combination of functors/templating so that all the pieces fit together, but it's not so bad now.

It's always a pain to do this.

I did not add integer tests (or std::vector tests). I guess it's important and I should do some thinking, but I might need to add extra lists of good/bad integer values. Rounding the good/bad double values won't always work (cause rounding a good value might give you an invalid value).

Right---it needs an extra list of good/bad integer values. And yes, this is important in that not having integers compile properly has bit us many times in the past. We'll have to make sure that the tests in Stan itself at least instantiate the integer cases.

"vector_rng_test_helper.hpp" is starting to get pretty verbose with all its little helpers and functions in it. Should I be breaking this out into pieces? If so, any suggestions on what needs broken out?

Always break into pieces if you can.

I imagine this stuff is going to keep growing as the rest of the distribution things get added, so maybe it's worth waiting to see if any other big structural changes come along before we go through the process of breaking things out.

Everything will keep growing, but that shouldn't freeze us in the meantime.

bbbales2 · 2017-10-05T15:59:17Z

Looks like the tests are passing, if it finishes, this is ready to review.

Questions:

I'm having trouble with the quantiles computed by Boost for the Skew Normal distribution (https://github.com/bbbales2/math/blob/d2281d17094e617148d763f8369354ec1627a852/test/unit/math/prim/mat/prob/skew_normal_test.cpp#L41)

It's really fragile. For instance the parameters 2, 1, -2.5 (in that order) cause some values of the quantiles to return -infs which screw up the tests. This is true for the non-vectorized non-integer tests as well. I can either try to improve the quantile computation, or I can just cherry pick some values that Boost can handle (currently what I've done).

I moved the recursive template stuff into a separate file (https://github.com/bbbales2/math/blob/d2281d17094e617148d763f8369354ec1627a852/test/unit/math/prim/scal/meta/apply_template_permutations.hpp#L116). I called it apply cause I think it'd be straightforward to make it match the apply semantics. It doesn't actually match them now, basically cause apply itself is a c++17 thing (but could be implemented in c++11 as I understand it). This could also be wiggled around to support things other than 3 template parameter functions.

If this looks like something that'd be useful for other tests, and it's obvious what necessary modifications it'd need, we can finish breaking it out (make it match the apply semantics + support other numbers of template arguments somehow). Otherwise, I'll just leave it as is (kinda special purpose for the vectorization stuff).

bob-carpenter · 2017-10-05T18:09:31Z

For (1), Cherry pick results for now. File an issue with Boost if you have examples that fail. (2) sounds good---we have a lot of test harness stuff that could stand to be consolidated.

bbbales2 · 2017-10-08T17:22:29Z

This is ready to be checked

syclik · 2017-10-22T03:01:43Z

@bob-carpenter, I think @bbbales2 has made all the requested changes. Want to make sure and approve after you're done?

bob-carpenter · 2017-10-22T06:29:29Z

Not everything got fixed. I don't have time to finish the next review now---I'm only a couple of my comments in but I should be able to get to it over the next few days.

bbbales2 · 2017-10-22T18:22:24Z

Cool beans, whenever is good. It'll be good to get this rolling again.

bob-carpenter · 2017-10-22T23:50:59Z

I'll take another pass as soon as I can. Sorry for the delay.

bob-carpenter

Most of these changes are optional other than doc to indicate that it's really scalar/standard vector/Eigen vector types.

This was super helpful in understanding some of these distributions---I hadn't looked closely enough at the code before.

bob-carpenter · 2017-10-22T06:47:35Z

stan/math/prim/scal/prob/double_exponential_rng.hpp

+
+      variate_generator<RNG&, uniform_01<> > rng_unit_01(rng, uniform_01<>());
+      for (size_t n = 0; n < N; n++) {
+        double laplaceRN = rng_unit_01();


[optional fix]

What I was suggesting in the original comment was something like this

double z = rng_unit_01(); double sign = bernoulli(0.5) ? 1 : -1; output[n] = mu_vec[n] - sign * sigma_vec[n] * log(z)

A slightly more efficient version would be

double z = uniform(-1, 1); output[n] = mu_vec[n] - sign(z) * sigma_vec[n] * log(abs(z))

For sign(), see: http://www.boost.org/doc/libs/1_62_0/libs/math/doc/html/math_toolkit/sign_functions.html

For uniform(-1, 1), see: http://www.boost.org/doc/libs/1_60_0/boost/random/uniform_real_distribution.hpp

For bernoulli(0.5), see: http://www.boost.org/doc/libs/1_60_0/boost/random/bernoulli_distribution.hpp

I got there through the version here with algebra, but it turns out to be pretty close to the Wikipedia version, only using uniform(-1, 1) and the recognition that 1 - uniform(-1, 1) has the same distribution as uniform(-1, 1) and the same sign randomization.

https://en.wikipedia.org/wiki/Laplace_distribution#Generating_random_variables_according_to_the_Laplace_distribution

bob-carpenter · 2017-10-24T19:11:27Z

stan/math/prim/scal/prob/exp_mod_normal_rng.hpp

-                       double sigma,
-                       double lambda,
+    /**
+     * Return a pseudorandom Exponentially modified normal variate for the


[optional fix]

I'd rather not capitalize anything other than proper names and names of distributions (not named after people) don't count.

bob-carpenter · 2017-10-24T19:13:00Z

stan/math/prim/scal/prob/exp_mod_normal_rng.hpp

+     * given location, scale, and inverse scale using the specified random
+     * number generator.
+     *
+     * mu, sigma, and lambda can each be either scalars or vectors. All vector


Can't they be scalars, standard vectors, or Eigen vectors?

bob-carpenter · 2017-10-24T19:13:36Z

stan/math/prim/scal/prob/exp_mod_normal_rng.hpp

+     * @return Exponentially modified normal random variate
+     * @throw std::domain_error if mu is infinite, sigma is nonpositive,
+     * or lambda is nonpositive
+     * @throw std::invalid_argument if vector arguments are not the same length


So I think this should say "container arguments" where you have "vector arguments".

bob-carpenter · 2017-10-24T19:14:50Z

stan/math/prim/scal/prob/exp_mod_normal_rng.hpp

-        + exponential_rng(lambda, rng);
-    }
+      size_t N = max_size(mu, sigma, lambda);
+


[optional fix]

I'd remove all the vertical space that's not absolutely necessary for readability. And that includes braces around for loops iwth single statements. Vertical space is precious!

bob-carpenter · 2017-10-24T19:19:39Z

stan/math/prim/scal/prob/logistic_rng.hpp

@@ -31,12 +42,25 @@ namespace stan {

      check_finite(function, "Location parameter", mu);
      check_positive_finite(function, "Scale parameter", sigma);
+      check_consistent_sizes(function,
+                             "Location parameter", mu,


[optional]

same comment here about packing arguments as much as possible

bob-carpenter · 2017-10-24T19:20:54Z

stan/math/prim/scal/prob/skew_normal_rng.hpp

+      VectorBuilder<true, double, T_loc, T_scale, T_shape> output(N);
+
+      variate_generator<RNG&, normal_distribution<> >
+        norm_rng(rng, normal_distribution<>(0.0, 1.0));


[optional]

In general, we prefer 0 and 1 to 0.0 and 1.0 (yet another thing in Google style not caught by cpplint)

bob-carpenter · 2017-10-24T19:22:04Z

stan/math/prim/scal/prob/skew_normal_rng.hpp

+        norm_rng(rng, normal_distribution<>(0.0, 1.0));
+      for (size_t n = 0; n < N; n++) {
+        double r1 = norm_rng(),
+          r2 = norm_rng();


[optional]

I'd prefer to have one declaration per line. And of course, tighten up vertical spaces!

bob-carpenter · 2017-10-24T19:22:24Z

stan/math/prim/scal/prob/student_t_rng.hpp

+     * Return a pseudorandom student-t variate for the given degrees of freedom,
+     * location, and scale using the specified random number generator.
+     *
+     * nu, mu, and sigma can each be either scalars or vectors. All vector


same fix here as everywhere about scalar/standard vector/Eigen vector.

bob-carpenter · 2017-10-24T19:24:57Z

test/unit/math/prim/mat/prob/VectorRNGTestRig.hpp

+                   std::vector<double> good_p2,
+                   std::vector<int> good_p2_int,
+                   std::vector<double> bad_p2,
+                   std::vector<int> bad_p2_int) : N_(N), M_(M),


[optional]

Put the : on a new line. You also don't want to waste horizontal space like this.

…d_continuous

bbbales2 · 2017-10-25T19:08:57Z

Can't they be scalars, standard vectors, or Eigen vectors?
So I think this should say "container arguments" where you have "vector arguments".

How about I change "vector" to "vector type" (like in https://github.com/stan-dev/stan/blob/develop/src/stan/lang/function_signatures.h#L10). This is how I did it in the normal_rng (must have just not copied it to the other _rngs yet):
mu and sigma can each be either a scalar or vector type (the ones that scalar_seq_view knows about). Any vector inputs must be the same length.

I'd like an easy way to refer to vector-like objects in a vague MATLAB/R this-is-vectorized kind of way.

same comment here about packing arguments as much as possible

Sure thing, this should all be pretty easy. I'll go through and see how much I can compress things.

bob-carpenter · 2017-10-25T19:24:58Z

How about "standard vector, Eigen vector, and Eigen row vector"? That's explicit and not too long. The reason to avoid reference to other classes is to preserve encapsulation---those other classes may change without this one changing.

…

On Oct 25, 2017, at 3:08 PM, Ben Bales ***@***.***> wrote: Can't they be scalars, standard vectors, or Eigen vectors? So I think this should say "container arguments" where you have "vector arguments". How about I change "vector" to "vector type" (like in https://github.com/stan-dev/stan/blob/develop/src/stan/lang/function_signatures.h#L10). This is how I did it in the normal_rng (must have just not copied it to the other _rngs yet): mu and sigma can each be either a scalar or vector type (the ones that scalar_seq_view knows about). Any vector inputs must be the same length. I'd like an easy way to refer to vector-like objects in a vague MATLAB/R this-is-vectorized kind of way. same comment here about packing arguments as much as possible Sure thing, this should all be pretty easy. I'll go through and see how much I can compress things. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

…torized_rngs_unbounded_continuous

…king

…ing locally) (Issue 45)

bbbales2 · 2017-10-28T22:37:16Z

I gotta ask for help on this one. I'm not sure what's going on, but it feels fishy. @seantalts or @syclik -- you guys might recognize something here -- it doesn't really have much to do with the actual pull.

The first problem I had was the scalar tests for double_exponential_rng were failing when the vector ones weren't.

I found two ways to fix it (and make the scalar double_exponential tests pass).

Replace log with std::log (9bf8a0e).

The effect I was seeing without this is that log(abs(z)) was going to -Inf, regardless of what z was. It felt like double was being cast to int, but I don't know if that should happen.

My guess was that it was only seeing the log definition here: https://github.com/stan-dev/math/blob/develop/stan/math/prim/scal/fun/log.hpp ? Which is only for ints? And for some reason wasn't seeing the usual std::log definition? But this doesn't explain why the vector tests worked.

Replace the stan/math/prim/scal.hpp include with stan/math/prim/mat.hpp

Both of these feel like wrong sorts of fixes. Any idea what could be going on?

seantalts · 2017-10-28T22:51:29Z

I took a 5 minute look and have only one possible suggestion - the old code had a using std::log; statement - was that present still when you saw the -Inf? It seems from my brief reading of it like that using statement is necessary (because as you point out it can only find the log(int) overload for that unqualified name and would thus try implicit conversions).

bbbales2 · 2017-10-28T22:56:32Z

@seantalts Oh wow, thanks! Problem solved. I didn't notice that.

Yeah I was wondering how abs was working but using std::abs is right there too. I guess something must just be leaking the the mat.hpp header that is accidentally making this work for the vector case when it shouldn't.

…Removed reference to stan::math::sign (Issue 45)

bob-carpenter · 2017-10-29T14:17:03Z

We don't need to do it for this pull request, but we really need to start cleaning up our includes and namespaces. Developers keep trying to sneak in global using statements, which should be caught by cpplint but isn't. That's where leaks come from.

When you have

  using std::log;
  log(3.2);  // std::log() available
  log(1);  // std::log() available
  std::log(3.2);  // std::log() forced
  std::log(1);  // std::log() forced

The using statement just the function log() (as defined in std::log) into namespace making it a candidate for resolution. The most specific resolution will be used and it won't compile if there's not a unique most-specific match.

So wht happens when you have std::log(x) that doesn't happen when you have using std::log; log(x)?

bob-carpenter · 2017-10-29T14:18:01Z

I should've added that if you're in namespace stan { namespace math { ... } } context then all of stan::math is automatically available.

bbbales2 · 2017-10-29T16:16:20Z

So wht happens when you have std::log(x) that doesn't happen when you have using std::log; log(x)?

Both of those should work. I'd messed up and removed the using std::log in one of my commits (this is what Sean pointed out). I switched to being explicit about std::log and std::abs.

On a related note, stan::math::log in prim doesn't seem to support doubles. stan::math::log in rev does (so the autodiff works). Is this considered a bug? If prim doesn't support what rev does?

bbbales2 · 2017-10-30T17:42:16Z

This should be good to go.

seantalts · 2017-11-01T20:26:17Z

(Please ignore the 3rd failing check - still working out the kinks)

…torized_rngs_unbounded_continuous

bbbales2 · 2017-11-07T01:44:31Z

Fixed the merge conflicts with the string stuff, this is good to go again.

bob-carpenter

This all looks great. The only change needed now is to put all the tests for illegal arguments to come before any work gets started allocating, etc. After that, it'll be fast to merge. All the math and container wrangling looks just fine.

bob-carpenter · 2017-11-13T22:22:50Z

stan/math/prim/scal/prob/cauchy_rng.hpp

+      scalar_seq_view<T_scale> sigma_vec(sigma);
+      size_t N = max_size(mu, sigma);
+      VectorBuilder<true, double, T_loc, T_scale> output(N);
+
      check_finite(function, "Location parameter", mu);


Checks should go first before any work. In general, try to put exceptional/odd/edge-case branches first so that when you're by them, you can scan the main branch of the program together. Would you mind fixing this on all of these? Sorry for not catchign that earlier.

bbbales2 · 2017-11-14T05:19:45Z

Well you didn't see it before cause in the last commit when I was cleaning up whitespace n' such I just moved all the variable declarations to be in one blob :P. I've moved them below the argument checks now. Should be good to go if the tests pass!

bob-carpenter · 2017-11-14T07:20:52Z

Thanks for moving up the argument validation tests. I approved (don't know why I'm saying that since it's obvious in the interface).

Changed int to size_t to avoid signed comparison warning (Issue 45)

fa7187b

Merge remote-tracking branch 'stan/develop' into feature/issue-45-vec…

270c96a

…torized_rngs_unbounded_continuous

bob-carpenter requested changes Sep 27, 2017

View reviewed changes

bbbales2 added 3 commits September 29, 2017 11:22

Merge remote-tracking branch 'stan/develop' into feature/issue-45-vec…

9a85e8c

…torized_rngs_unbounded_continuous

Moved around & symbols and if statements. Avoided cast w/ templating

ba6c624

Added int and std::vector<int> tests (Issue 45)

d2281d1

bob-carpenter requested changes Oct 24, 2017

View reviewed changes

seantalts mentioned this pull request Oct 24, 2017

Issue 123 #643

Merged

3 tasks

Merge branch 'develop' into feature/issue-45-vectorized_rngs_unbounde…

427d316

…d_continuous

bbbales2 added 6 commits October 25, 2017 20:49

Cleaned up spacing, changed docs to make interface clearer (Issue 45)

8e83e4f

Fixed cpplint issues in test (Issue 45)

ee338ff

Merge remote-tracking branch 'stan/develop' into feature/issue-45-vec…

25daa7c

…torized_rngs_unbounded_continuous

Swapped std::log for log in double_exponential test to get things wor…

9bf8a0e

…king

Made a couple fixes to get Jenkins tests to pass (errors not reproduc…

aa207eb

…ing locally) (Issue 45)

Fix cpplint (Issue 45)

4517668

Fixed references to std::log and std::abs in double_exponential_rng. …

eff2de9

…Removed reference to stan::math::sign (Issue 45)

Merge remote-tracking branch 'stan/develop' into feature/issue-45-vec…

ddb60d9

…torized_rngs_unbounded_continuous

bob-carpenter reviewed Nov 13, 2017

View reviewed changes

Moved around variable declarations below argument checks (Issue 45)

47cdfc9

bob-carpenter approved these changes Nov 14, 2017

View reviewed changes

seantalts merged commit 4a27d5b into stan-dev:develop Nov 16, 2017

bbbales2 mentioned this pull request Jun 27, 2018

Vectorized a bunch of the discrete rngs #833

Merged

3 tasks

rok-cesnovar mentioned this pull request Mar 23, 2021

skew_normal_rng overflowing #527

Closed

Vectorized unbounded continuous random number generators (Issue 45) #622

Vectorized unbounded continuous random number generators (Issue 45) #622

Conversation

bbbales2 commented Sep 17, 2017

Submission Checklist

Summary:

Details

How to Verify:

Side Effects:

Documentation:

Copyright and Licensing

bbbales2 commented Sep 17, 2017

bob-carpenter commented Sep 18, 2017

bbbales2 commented Sep 18, 2017

bbbales2 commented Sep 21, 2017 • edited Loading

seantalts commented Sep 21, 2017

bbbales2 commented Sep 21, 2017

bbbales2 commented Sep 21, 2017

bbbales2 commented Sep 24, 2017

bbbales2 commented Sep 27, 2017

bob-carpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbbales2 commented Oct 1, 2017 • edited Loading

bob-carpenter commented Oct 2, 2017 via email

bbbales2 commented Oct 5, 2017

bob-carpenter commented Oct 5, 2017 via email

bbbales2 commented Oct 8, 2017

syclik commented Oct 22, 2017

bob-carpenter commented Oct 22, 2017

bbbales2 commented Oct 22, 2017

bob-carpenter commented Oct 22, 2017

bob-carpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbbales2 commented Oct 25, 2017

bob-carpenter commented Oct 25, 2017 via email

bbbales2 commented Oct 28, 2017

seantalts commented Oct 28, 2017

bbbales2 commented Oct 28, 2017

bob-carpenter commented Oct 29, 2017

bob-carpenter commented Oct 29, 2017

bbbales2 commented Oct 29, 2017

bbbales2 commented Oct 30, 2017

seantalts commented Nov 1, 2017

bbbales2 commented Nov 7, 2017

bob-carpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbbales2 commented Nov 14, 2017

bob-carpenter commented Nov 14, 2017

bbbales2 commented Sep 21, 2017 •

edited

Loading

bbbales2 commented Oct 1, 2017 •

edited

Loading