Reduce Boost dependency footprint - #3405
Conversation
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
My coworker @DiamonDinoia has encouraged me to stop using this generator as well (can be a separate PR) |
Interesting! What's the recommended replacement? If we're looking at changing the RNG it would probably be best if we did the With the feature freeze coming up did you want to do the RNG change this release or hold off on the |
|
@vigna has a great page to guide the choice: https://prng.di.unimi.it/ look for how to choose a PRNG. If I were to implement the PRNG from scratch I would implement one of xoshiro256++/xoshiro256** which are 10 liners. For example in c++: https://github.com/DiamonDinoia/simdrng/blob/main/include/simdrng/xoshiro_scalar.hpp If I can add a dependency I would use ChaCha20, AES in counter mode relies on having AES implemented in hw which not all platforms have. And I think Stan aims to support many no? I personally implemented those into my package here https://github.com/DiamonDinoia/simdrng I am still developing and cleaning up a few things. But I wanted to take the change to make a product placement. This package tests some generators and share some findings: https://github.com/vigna/modlin-rs |
|
If you implement any of those generators, I am happy to help review. |
SteveBronder
left a comment
SteveBronder
left a comment
There was a problem hiding this comment.
Few changes and I need to look over the ring buffer impl still
| double variance = (y.array() - y.mean()).matrix().squaredNorm() / y.size(); | ||
|
|
||
| accumulator_set<double, stats<variance>> acc; | ||
| for (int n = 0; n < y.size(); ++n) { | ||
| acc(y(n)); | ||
| } | ||
|
|
||
| acov = acov.array() * boost::accumulators::variance(acc); |
There was a problem hiding this comment.
We should use a little variance function like the below that uses welford's algorithm so we can be a bit more precise
| * @return the tokens found in `s` | ||
| */ | ||
| inline std::vector<std::string> split(const std::string& s, | ||
| const std::string& delims, |
There was a problem hiding this comment.
Bit of code golf, but I think you can shorten this a good bit
| if (i > 0) | ||
| result += sep; |
There was a problem hiding this comment.
| if (i > 0) | |
| result += sep; | |
| if (i > 0) { | |
| result += sep; | |
| } |
| */ | ||
| inline std::string join(const std::vector<std::string>& parts, | ||
| const std::string& sep) { | ||
| std::string result; |
There was a problem hiding this comment.
You will be allocating at least parts.size() sep + parts[0]
| std::string result; | |
| std::string result; | |
| result.reserve(parts[0].size() + parts.size()); |
| inline void replace_first(std::string& s, const std::string& target, | ||
| const std::string& replacement) { |
There was a problem hiding this comment.
| inline void replace_first(std::string& s, const std::string& target, | |
| const std::string& replacement) { | |
| inline void replace_first(std::string_view& s, const std::string_view& target, | |
| const std::string_view& replacement) { |
| double mx = x.head(M).mean(); | ||
| double my = y.head(M).mean(); | ||
| return ((x.head(M).array() - mx) * (y.head(M).array() - my)).sum() | ||
| / (M - 1.0); |
There was a problem hiding this comment.
Like the variance I think it would be nice to add a welford style estimator for covariance here
| std::vector<double> probs_vec(probs.data(), probs.data() + probs.size()); | ||
| std::vector<double> q = stan::math::quantile(x, probs_vec); | ||
| return Eigen::Map<const Eigen::VectorXd>(q.data(), q.size()); |
There was a problem hiding this comment.
This needs to be cast to a vector to use quantile? Could we update that function to also accept Eigen vectors?
5 participants
Submission Checklist
./runTests.py src/test/unitmake cpplintSummary
Related to this Math PR, this PR removes/replaces non-math Boost includes with std-equivalents.
The following includes are removed:
boost/random: Replaced with<random>equivalents, expected test values also updatedboost/algorithm: Only basic string manip was needed, these have been implemented understring_utils.hppinsteadboost/regex: Only used to check whether variable name was valid, pattern was very simple so just usedstd::isalpha&std::isalnumboost/accumulators: Replaced with Eigen functionsNow the only non-math Boost include is
boost/random/mixmax. Theboost/accumulatorsremoval in particular is a big win, since that pulled in bothboost/mplandboost/fusion- these two libraries alone are ~20mb!Let me know if it would be easier to review if I split the changes into separate PRs
Intended Effect
Reduce amount of Boost libraries/headers needed by Stan
How to Verify
Side Effects
Changing the rng usage from boost -> std will return different values from same seed.
Documentation
N/A
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): Andrew Johnson
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: