Skip to content

Improve erf/expm1/expint coverage. #1111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 54 commits into
base: develop
Choose a base branch
from
Open

Conversation

jzmaddock
Copy link
Collaborator

In the expm1 case, tighten up error handling and testing.

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 99.33775% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.36%. Comparing base (5459e14) to head (ba6838c).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1111      +/-   ##
===========================================
+ Coverage    93.77%   94.36%   +0.58%     
===========================================
  Files          772      774       +2     
  Lines        61352    59754    -1598     
===========================================
- Hits         57535    56386    -1149     
+ Misses        3817     3368     -449     
Files Coverage Δ
.../boost/math/special_functions/detail/bessel_ik.hpp 100.00% <100.00%> (ø)
.../boost/math/special_functions/detail/bessel_j1.hpp 100.00% <ø> (ø)
.../boost/math/special_functions/detail/bessel_k0.hpp 100.00% <ø> (ø)
.../boost/math/special_functions/detail/bessel_k1.hpp 100.00% <ø> (ø)
...ost/math/special_functions/detail/igamma_large.hpp 100.00% <ø> (+66.88%) ⬆️
...ost/math/special_functions/detail/lgamma_small.hpp 100.00% <100.00%> (+40.63%) ⬆️
.../boost/math/special_functions/detail/polygamma.hpp 91.38% <ø> (-0.64%) ⬇️
...h/special_functions/detail/unchecked_factorial.hpp 99.10% <ø> (-0.90%) ⬇️
include/boost/math/special_functions/erf.hpp 100.00% <ø> (+0.45%) ⬆️
include/boost/math/special_functions/expint.hpp 100.00% <ø> (+2.03%) ⬆️
... and 29 more

... and 8 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 5459e14...ba6838c. Read the comment docs.

Fixed Conflicts:
	include/boost/math/special_functions/detail/bessel_ik.hpp
	include/boost/math/special_functions/detail/bessel_j1.hpp
	include/boost/math/special_functions/detail/bessel_k0.hpp
	include/boost/math/special_functions/detail/bessel_k1.hpp
	include/boost/math/special_functions/detail/igamma_large.hpp
	include/boost/math/special_functions/detail/lgamma_small.hpp
	include/boost/math/special_functions/expint.hpp
	include/boost/math/special_functions/expm1.hpp
	include/boost/math/special_functions/gamma.hpp
	include/boost/math/special_functions/log1p.hpp
	include/boost/math/special_functions/trigamma.hpp
	test/Jamfile.v2
	test/test_expint.cpp
Add support for <cstdfloat> types as well.
Extend tests for better coverage.
Disable MP expm1 from coverage check.
@jzmaddock
Copy link
Collaborator Author

@mborland @ckormanyos @NAThompson This PR is now good to go and completes code coverage improvements as far as G for gamma.hpp. There are a lot of accumulated changes here, so I'll hold off for a few days at least in case there are any comments...

Also not sure what's happened to the drone CI runner: it seems to be terminally broken right now?

@mborland
Copy link
Member

@sdarwin have something gone sideways with our drone config? It looks like we have quite a few runs over the past several days that aren't picking up? There are some like this that seem to have completely crashed before the retry step in the clone phase: https://drone.cpp.al/boostorg/math/2225/193/1

Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

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

This all looks good to me John. Once it's in I can run it through the actual CUDA testing to see if there's any changes that need to be made there. I think the only thing I saw with the replacement of BOOST_MATH_FP_XXX with the standard FP_XXX macros. No matter since Scipy is now pinned against specific versions instead of tracking develop.

@jzmaddock
Copy link
Collaborator Author

This all looks good to me John. Once it's in I can run it through the actual CUDA testing to see if there's any changes that need to be made there. I think the only thing I saw with the replacement of BOOST_MATH_FP_XXX with the standard FP_XXX macros. No matter since Scipy is now pinned against specific versions instead of tracking develop.

Where was this? I didn't think I'd touched tools/config.hpp?

//
if((boost::math::fpclassify)(prefix) == (int)BOOST_MATH_FP_INFINITE)
return policies::raise_overflow_error<T>("boost::math::detail::full_igamma_prefix<%1%>(%1%, %1%)", "Result of incomplete gamma function is too large to represent.", pol);
if((boost::math::fpclassify)(prefix) == (int)FP_INFINITE)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if((boost::math::fpclassify)(prefix) == (int)FP_INFINITE)
if((boost::math::fpclassify)(prefix) == (int)BOOST_MATH_FP_INFINITE)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

I'll do that before merging.

Unrelated question: should BOOST_MATH_FP_* map to the std values of FP_* ?

Copy link
Member

Choose a reason for hiding this comment

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

They do: https://github.com/boostorg/math/blob/develop/include/boost/math/tools/config.hpp#L780-L784, except in the case of NVRTC where they map to the conventional values: https://github.com/boostorg/math/blob/develop/include/boost/math/tools/config.hpp#L830-L834, which works since boost::math::fpclassify returns the BOOST_MATH_XXX macro values

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.

2 participants