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
There was a problem hiding this comment.
Interesting! I was looking into the speed/accuracy tradeoff from switching from vectorised Eigen to welford's, and it's showing the Eigen vectorised impl as being more accurate still: https://godbolt.org/z/G33637P6r.
I've also attached the version that did a timing comparison across a larger range as well.
I'm not overly-confident that Claude isn't missing something obvious though, are there any particular edge-cases or examples where you've seen the welford benefits that I can test with? I was always under the impression that the main motivator was for online/streaming cases rather than with a fully-materialised case.
There was a problem hiding this comment.
We do have existing welford estimators for variance and covariance in the Math library that I could switch to here and for the covariance below:
There was a problem hiding this comment.
Relative to @andrjohns' file variance_accuracy.cpp.
-
eigen_twopass is generally more accurate, but can fail in extreme cases. John Cook has a post about this. It's not clear how common such cases are.
-
I've always written the variance calculations for welford_seq as
m2 += -m2 * inv + delta * delta * inv * (1 - inv);and then returnm2 * n / (n - 1);, which proves a bit more accurate than welford_seq, and otherwise follows the pattern of the bullet point above.
| * @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?
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: |
|
Presumably we can also remove some more pieces of boost and shrink the cmdstan tarball size |
Yeah we can drop a ton! I've got one PR after this one to remove the usages in
The The other 16 boost libs (~19mb) are all pulled in by MPI, so any non-mpi model would now only pull in the |
|
(that's for the tarball, there are a few more libs used for tests in the |
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: |
7 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: