stan-dev / math

The Stan Math Library is a C++ template library for automatic differentiation of any order using forward, reverse, and mixed modes. It includes a range of built-in functions for probabilistic modeling, linear algebra, and equation solving.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
723 stars 183 forks source link

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

Open andrjohns opened 1 month ago

andrjohns commented 1 month ago

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

WardBrian commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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)