Update RNG tests to use mixmax RNG - declare stan::rng_t in Math library instead

[andrjohns]

Summary

As seen over in this Stan issue, it looks like the new mixmax RNG has some edge cases. This PR updates our tests to use the same RNG - just in case there are any others.

I also think we should have the common RNG declaration (stan::rng_t) in the Math library instead, so that any changes are implicitly tested - but also happy to revert if others disagree

Tests

N/A

Side Effects

N/A

Release notes

Update tests to use mixmax RNG, declare stan::rng_t in Math library

Checklist

• Copyright holder: Andrew Johnson

  The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the 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/)

• [] the basic tests are passing

  • unit tests pass (to run, use: ./runTests.py test/unit)
  • header checks pass, (make test-headers)
  • dependencies checks pass, (make test-math-dependencies)
  • docs build, (make doxygen)
  • code passes the built in C++ standards checks (make cpplint)

• the code is written in idiomatic C++ and changes are documented in the doxygen

• the new changes are tested

[WardBrian]

Because the downstream usages do not directly call the constructor of the rng type directly and instead call the utility function that accepts a seed and chain ID, I don’t see the value of moving the typedef here (if anything, it might give the wrong idea?)

[andrjohns]

Because the downstream usages do not directly call the constructor of the rng type directly and instead call the utility function that accepts a seed and chain ID, I don’t see the value of moving the typedef here (if anything, it might give the wrong idea?)

It was more to make sure that we were always using the same RNG in Stan and the Math library tests, but I'm not particularly invested either way. I'll revert for now to just specifying boost::random::mixmax since this was more about running the tests, and the RNG typedef location can be a separate discussion

[WardBrian]

I think it's fine to want to use the same RNG for testing here as is used in Stan (though historically this wasn't done, and I could also see arguments for not wanting to do this)

But it's worth noting that

I also think we should have the common RNG declaration (stan::rng_t) in the Math library instead, so that any changes are implicitly tested

Isn't really true, at least in the sense that it would not have given us any earlier notice on this issue or anything else that was specific to certain seeds, because as far as I can tell we either default construct the RNGs used in tests or use a pretty small list of seeds (e.g. 1234)

[andrjohns]

@spinkney another LKJ question for you. This test is now failing on the derivative comparison below:

EXPECT_FLOAT_EQ(2.5177896, stan::math::lkj_corr_log(Sigma, eta).d_);

Would you know where the fixed value 2.5177896 came from? (I know this wasn't a test you wrote, but I figured you probably understand this better than I do!)

[spinkney]

No, this doesn't seem right. The derivative is wrt to eta, right? And eta is changing. I think the derivative should be

eta <- 0.3
d <- 4

d_eta <- -(d - 2) * digamma((d - 1) / 2 + eta) + 2 * digamma(d/2 + eta - 1) - 2 * digamma(d + 2 * eta - 2) + log(4)
for (k in 2:(d-1)) {
  d_eta <- d_eta + digamma(eta + (d-1-k)/2)            
}
d_eta

[WardBrian]

Using an RNG in this test seems like it isn't necessary anyway. There is no randomness used, the value of eta is the same every time, we just avoid writing it down explicitly I guess?

If you replace the definition for eta with the line fvar<double> eta = 1.6294473838061094;, which is just what the result of the default-constructed mt19937 was giving before, the test passes again.

[spinkney]

That makes more sense and the rng should be removed.