Skip to content
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

Update pre-computed constexpr Gauss and Gauss-Kronrod constants #1079

Merged
merged 5 commits into from
Feb 9, 2024

Conversation

jzmaddock
Copy link
Collaborator

to always store literal types.
Fixes: #1077.

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ca29a70) 85.21% compared to head (b2b839a) 84.90%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1079      +/-   ##
===========================================
- Coverage    85.21%   84.90%   -0.31%     
===========================================
  Files          879      880       +1     
  Lines        66880    67275     +395     
===========================================
+ Hits         56989    57120     +131     
- Misses        9891    10155     +264     
Files Coverage Δ
include/boost/math/quadrature/gauss.hpp 65.03% <100.00%> (-25.10%) ⬇️
include/boost/math/quadrature/gauss_kronrod.hpp 49.31% <100.00%> (-45.90%) ⬇️
test/git_issue_1075.cpp 100.00% <100.00%> (ø)

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 ca29a70...b2b839a. Read the comment docs.

@SteveBronder
Copy link

SteveBronder commented Feb 1, 2024

How would you feel about something like the below? I wrote a is_constexpr_constructible() function that is used to deduce which gauss_detail to use. If Real's constant type is constructible from a long double at compile time (which I think is true for float, double, and long double) then it uses the one that is constexpr. Otherwise it uses one that constructs the values at runtime.

Looking at the version here I think something bad would happen across if we ran this once, cleaned up the arena, and tried running again. I'm always bad at the intricacies of static types in functions, but if they are declared once for the life of the program and we clean up the memory arena then I think the next time we run this the program would do UB accessing memory that has been cleaned up or reallocated.

https://godbolt.org/z/j1br4GTno

EDIT: updated link to compile with c++14

Also mark up anything that uses BOOST_MATH_HUGE_CONSTANT as unreachable by code coverage as gcov seems to have an issue with it.
Copy link

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

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

Just a few small comments. I'm not really following how this fixes #1077

include/boost/math/quadrature/gauss.hpp Outdated Show resolved Hide resolved
include/boost/math/quadrature/gauss.hpp Show resolved Hide resolved
include/boost/math/quadrature/gauss.hpp Outdated Show resolved Hide resolved
@jzmaddock
Copy link
Collaborator Author

I hope this should be good to go: @SteveBronder can you try this PR against your actual code?

@SteveBronder
Copy link

Thanks I'll give this a whirl on Monday!

@SteveBronder
Copy link

This works!

@NAThompson
Copy link
Collaborator

@jzmaddock : Looks like the build failure is unrelated-want to merge?

@jzmaddock
Copy link
Collaborator Author

Looks like the build failure is unrelated-want to merge?

Yup, merging...

@jzmaddock jzmaddock merged commit cbf6b96 into develop Feb 9, 2024
67 of 68 checks passed
@NAThompson NAThompson deleted the issue1075 branch February 9, 2024 18:18
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.

gauss_kronrod does not allow for non-literal types
3 participants