Skip to content

Reduce Boost dependency footprint - #3356

Open
andrjohns wants to merge 5 commits into
developfrom
boost-repl
Open

Reduce Boost dependency footprint#3356
andrjohns wants to merge 5 commits into
developfrom
boost-repl

Conversation

@andrjohns

Copy link
Copy Markdown
Collaborator

Summary

Including Boost headers outside of their Math library tends to pull in a surprising amount of other Boost headers.

For example, for the single lexical_cast call in init_threadpool_tbb() the total Boost footprint is:

  • Boost.Config
  • Boost.TypeTraits
  • Boost.Container
  • Boost.Core
  • Boost.Detail
  • Boost.Exception
  • Boost.ThrowException
  • Boost.Limits
  • Boost.Move
  • Boost.StaticAssert
  • Boost.Version
  • Boost.LexicalCast

This PR removes three different types of Boost dependencies:

  1. boost/lexical_cast (init_threadpool_tbb): Replaced with a std::from_chars + try/catch
  2. boost/optional (hypergeometric_2F1): Replaced with std::optional
  3. boost/random (_rng functions): Replaced with std:: equivalents (e.g., std::bernoulli_distribution bernoulli_rng)

Tests

N/A - Existing tests should still pass.

Note that this triggered an edge-case with the expected-value (chi-square) testing for distributions, where too few values were sampled for particular bins. The testing util was updated to just merge bins if the expected counts were too low.

Side Effects

Possibility of changes to existing _rng outputs with same seed value

Release notes

Replaced Boost dependencies with standard library equivalents

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

Copy link
Copy Markdown
Member

Funny, I was just talking to someone today about decreasing our reliance on boost/random

Can we add any new libraries to the trim list

math/lib/upgrade-boost.sh

Lines 117 to 120 in cc6ba8b

# unused libraries, focusing on larger ones
UNUSED="leaf nowide pfr json static_string stl_interfaces phoenix msm redis mysql log \
geometry hana asio multiprecision beast qvm gil xpressive process parser charconv \
wave atomic polygon metaparse interprocess yap thread compute url test proto"

after this?

Comment on lines +180 to +185
// alpha < 1 makes this distribution's tail very sparse, so most of the K
// bins have expected counts far below N; a single sample landing in one
// of them can dominate the statistic. Pool bins so each group has an
// expected count of at least 5, per standard chi-square goodness-of-fit
// practice. Grouping is based only on the theoretical expected counts
// (not the realized samples), so it doesn't bias the test.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add this hook to your claude and it stops it from doing these multi line comments

https://gist.github.com/SteveBronder/8c2f141fed4511447a8ad5939157857e

Generally I have seen that if you let it start writing a lot of these it uses them as an excuse to do bad code. The comment can also be wrong since it is untested which can lead to other agents down the road getting confused here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh super cool, immediately stolen

@SteveBronder SteveBronder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@SteveBronder

Copy link
Copy Markdown
Collaborator

From the tests it seems like the changes in stan have to happen first before we can merge this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants