Skip to content

Conversation

@jzmaddock
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 93.58974% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 93.69%. Comparing base (085da47) to head (ebe7b36).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1099      +/-   ##
===========================================
+ Coverage    92.61%   93.69%   +1.08%     
===========================================
  Files          770      770              
  Lines        63889    62824    -1065     
===========================================
- Hits         59170    58866     -304     
+ Misses        4719     3958     -761     
Files Coverage Δ
include/boost/math/special_functions/beta.hpp 100.00% <100.00%> (+3.69%) ⬆️
include/boost/math/special_functions/binomial.hpp 100.00% <100.00%> (+16.66%) ⬆️
.../boost/math/special_functions/detail/bessel_y0.hpp 100.00% <ø> (ø)
...de/boost/math/special_functions/detail/erf_inv.hpp 100.00% <ø> (+4.13%) ⬆️
include/boost/math/special_functions/digamma.hpp 100.00% <ø> (+10.05%) ⬆️
include/boost/math/special_functions/ellint_1.hpp 100.00% <100.00%> (+1.69%) ⬆️
include/boost/math/special_functions/ellint_2.hpp 100.00% <100.00%> (+2.38%) ⬆️
include/boost/math/special_functions/ellint_3.hpp 100.00% <100.00%> (+7.08%) ⬆️
include/boost/math/special_functions/ellint_d.hpp 100.00% <100.00%> (+7.84%) ⬆️
include/boost/math/special_functions/ellint_rc.hpp 100.00% <100.00%> (+13.79%) ⬆️
... and 23 more

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 085da47...ebe7b36. Read the comment docs.

@ckormanyos
Copy link
Member

ckormanyos commented Feb 26, 2024

Hi John (@jzmaddock),

I've been following this work with great interest and admiration. In recent red CI there's some of this kind of stuff related to BOOST_MATH_BERNOULLI_UNTHREADED

Sorry I did not catch all of this in my own bare-metal standalone tests.

But there are several compiler switches which might seemingly do the same thing.

  • BOOST_MATH_BERNOULLI_UNTHREADED is this really needed for Bernoulli exclusively?
  • BOOST_MATH_HAS_THREADS OK, Boost.Math has threads.
  • BOOST_MATH_DISABLE_THREADS hmmm... Does Boost.Math also have threads and then disable them?
  • BOOST_MATH_NO_ATOMIC_INT if Boost.Math does not have threads is is sane/safe to assume it also lacks atomics?

I could not wade through all this, but it seems like we could simplify some things as we move forward? Thoughts?

Cc: @mborland

@ckormanyos
Copy link
Member

ckormanyos commented Feb 26, 2024

I guess in my (maybe naive world), Boost.Math could have threads by default. Then one sinlge option to disable threads could replace all redundant of the other three options listed above?

Maybe I'm over-simplifying?

Cc: @jzmaddock and @mborland

@jzmaddock
Copy link
Collaborator Author

I could not wade through all this, but it seems like we could simplify some things as we move forward? Thoughts?

Unfortunately I fear probably not.

The Bernoulli numbers only have to deal with threads in the sense that the cache has to be generated in a thread safe way. And this only applies to the multiprecision case I believe. The most efficient way is to use atomics, but while is guaranteed, there is no guarantee that it contains anything useful (or efficient). Hence BOOST_MATH_NO_ATOMIC_INT does NOT imply no threads, just no efficient atomics.

Then we have platforms like Mingw/Cygwin which have no working std::thread or mutexes, from memory you can't even #include a thread related header without getting an error :(

So the options are:

  1. We have threads and atomic int's, so use these.
  2. We have threads, but no atomics, use mutexes (less efficient).
  3. We appear to have no threads - wohoo, all nice and simple! However, this may be very unsafe (see below).
  4. We have threads, but the user knows up front that they're not using them, so disables support with BOOST_DISABLE_THREADS for the fastest/simplest possible code. Then as (3).

I had to look up BOOST_MATH_BERNOULLI_UNTHREADED: when we have option (3) above, the default behaviour is to static_assert on the grounds that we don't know how to make things safe, but BOOST_MATH_BERNOULLI_UNTHREADED can be defined to suppress the error if the user knows what they're doing.

And finally, if I can remember to type BOOST_MATH_ASSERT instead of BOOST_ASSERT, I might actually get some of this green!

@ckormanyos
Copy link
Member

I could not wade through all this, but it seems like we could simplify some things as we move forward? Thoughts?

Unfortunately I fear probably not.

... and a whole bunch of very sensible information...

Yes John (@jzmaddock), I get it now. Somehow I was vaguely familiar with your points, that you have clarified perfeclty. Please disregard my noise and leave everything alone, with your comment that we all need to use BOOST_MATH_whatever consistently, as required (I had to deal with this one myself also on my metal benchmarks).

What a great result. Yet sorry about the noise.

Thx again.

Cc: @mborland

@mborland mborland mentioned this pull request Feb 28, 2024
@jzmaddock
Copy link
Collaborator Author

OK, this is good to go I hope, one failure is a network timeout. Will merge and then see if I can re-trigger multiprecision develop.

@jzmaddock jzmaddock merged commit d893223 into develop Feb 29, 2024
@NAThompson NAThompson deleted the improve_coverage_2 branch February 29, 2024 16:55
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.

3 participants