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

First batch changes double-fp-backend #1226

Merged
merged 7 commits into from
Dec 19, 2024
Merged

Conversation

ckormanyos
Copy link
Member

I need to make some tiny changes in the Math tests when testing the candidate double_fp_backend<double> as a multiprecision backend.

The min./max. exponents are really small (comparatively) for a multiple-precision type. I'm running the tests for double-double, which has min_exponent10 of $-291$ and max_exponent10 of $308$. So this causes a few over/underflows unexpectedly for a multiprecision type under test.

A second batch of similarly small, but more difficult-to-make changes will be needed. But I will do these in a second step, pending approval of the first, simpler batch.

Cc: @jzmaddock and @mborland

@ckormanyos
Copy link
Member Author

ckormanyos commented Dec 19, 2024

See also PR154 in GSoC repo

Cc: @sinandredemption and @cosurgi

@ckormanyos
Copy link
Member Author

ckormanyos commented Dec 19, 2024

One example of a change needed comes from the test point of the irregular Bessel function $K_{\nu}\left(x\right)$, for $x{\in}\mathbb{R}$.

We need to compute

$$ K_{-1000}\left(700\right)\text{,} $$

which internally needs to compute the value of

$$ e^{-700}{\approx}10^{-305}\text{.} $$

This number, however, underflows cpp_double_fp_backend<double> for 64-bit double, having min_exponent10 of $-291$.

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.83%. Comparing base (699d79b) to head (214e19c).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1226      +/-   ##
===========================================
- Coverage    93.83%   93.83%   -0.01%     
===========================================
  Files          657      657              
  Lines        55241    55241              
===========================================
- Hits         51836    51833       -3     
- Misses        3405     3408       +3     
Files with missing lines Coverage Δ
.../boost/math/special_functions/detail/bessel_k0.hpp 100.00% <ø> (ø)
test/cubic_roots_test.cpp 92.47% <100.00%> (ø)
test/linear_regression_test.cpp 100.00% <100.00%> (ø)
test/quartic_roots_test.cpp 100.00% <100.00%> (ø)
test/test_bessel_i.hpp 100.00% <ø> (ø)
test/test_bessel_j.hpp 100.00% <100.00%> (ø)

... and 1 file 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 699d79b...214e19c. Read the comment docs.

@ckormanyos
Copy link
Member Author

Hi John (@jzmaddock) and Matt (@mborland) there seem to be some tolerance issues in files like linear_regression_test.cpp on the clang-port to MSVC 2022.

I'm not qualified to "up" the relevant tolerances. Does anyone know if these failures are relevant? Is it just a tolerance issue? Or does something actually need to be done?

I'm not entirely sure, but I do not think my changes in areas like Bessel functions are related. But you never know in Math. Anyway, could you guys take a look? Thanks.

@mborland
Copy link
Member

Github updated the version of MSVC2022 and it caused three tests to fail consistently. I can nudge up the tolerance a bit because it's been consistent across runs instead of sporadic like normal tolerance problems.

@ckormanyos
Copy link
Member Author

I can nudge up the tolerance a bit

Should I just do that here while I'm on such a roll? If that's what's to be done, I can handle it. Thx Matt (@mborland)

@mborland
Copy link
Member

I can nudge up the tolerance a bit

Should I just do that here while I'm on such a roll? If that's what's to be done, I can handle it. Thx Matt (@mborland)

Please do

@cosurgi
Copy link

cosurgi commented Dec 19, 2024

Wow, @ckormanyos thank you very much for working on this. I am quite busy at the moment, so I only had a brief look, but please keep working. I don't remember much, but if it will be needed I will try to recall what I wrote in some of my code, when you will need it.

@ckormanyos
Copy link
Member Author

thank you very much for working on this

Thanks @cosurgi we are actually getting close. Conservatively, I think we can get the double-fp backend into summer 1.89. But I might even hope for April's 1.88. Let's see if we can get all the edge cases to behave. It's not that far off the mark now. I'd like to first get double-double stable and in, then in a second push charge on for quad.

@ckormanyos
Copy link
Member Author

I can nudge up the tolerance a bit

Should I just do that here while I'm on such a roll? If that's what's to be done, I can handle it. Thx Matt (@mborland)

Please do

Cycling now

@ckormanyos
Copy link
Member Author

ckormanyos commented Dec 19, 2024

OK guys, CI is green. Yet the first batch of changes is not quite ready.

For the type cpp_double_double, we still have to negotiate on:

  • the test of cyl_bessel_j(364, 35.5)
  • and the test of cyl_bessel_k(-1000, 700)

I consider the second point to be done. The sub-normal case is a bit more tricky and I still need to check if my zero-handling is correct in the backend. Then we'll see what's up on that test case.

Cc: @jzmaddock

Also @mborland the adjusted tolerances on the cases we mentioned are (perhaps liberal) but green.

@jzmaddock
Copy link
Collaborator

OK, I think I see the issue, min and max values are asymetric, so in bessel_k0.hpp:473 the line:

if(x < tools::log_max_value<T>())

Should actually be:

if(-x > tools::log_min_value<T>())

There will be other places as well... you caught me be lazy!! But that (along with any other similar mistakes) should fix things.

@ckormanyos
Copy link
Member Author

ckormanyos commented Dec 19, 2024

OK, I think I see the issue, min and max values are asymetric,

OK amazing catch John, let me run that thing through CI and see how it shakes out.

There will be other places as well...

It's really rather easy-going. I have a really short remaining-issue-list with one single point of ellint_3, a whole bunch of inv_gamma() with GIANT exponents (I can deal with these), a few things more on $K_{\nu}$. But that's pretty much it. So all in all, the generic-ness of Math/Multiprecision is simply awesome.

Let me see if we can dial in this double-trouble type ASAP.

@ckormanyos
Copy link
Member Author

ckormanyos commented Dec 19, 2024

Ah OK. John with the change in bessel_k0.hpp it is very close. I do, however, still need to

#define BOOST_NO_LIMITS_COMPILE_TIME_CONSTANTS

Otherwise I get the vanilla hard-coded values for logarithms of max/min values. And I'm fine, perfectly fine defining this (at least preliminarily) in the cpp_double_fp_backend main header file until we hash out the rest of the wrinkles.

Let me back out the changes in $K_{\nu}$ tests.

Then the only open point is:

$$ J_{364}\left(35{\frac{1}{2}}\right) $$

@ckormanyos
Copy link
Member Author

the only open point is

I mean in this PR. There will be batch2 of specfun changes needed to negotiate coming when we finish batch1.

@ckormanyos
Copy link
Member Author

ckormanyos commented Dec 19, 2024

OK I needed to correct two wrongly-placed constexpr attributes.

I need to move forward with this thing. So I just reverted the test-case-change on cyl_bessel_j. So the negotiation is in my view closed. If/when this thing cycles, I'd like to consider to merge it. Then I can charge ahead on with cpp_double_fp_backend...

@jzmaddock
Copy link
Collaborator

OK amazing catch John, let me run that thing through CI and see how it shakes out.

What I meant to say was - "I tend to make the same mistake over and over" so since all the Bessel rational approximations are pretty similar, there are probably other places where we have this bug whether or not we happen to have (un)fortunate test values in the (un)sweet spot that triggers the issue.

Ah OK. John with the change in bessel_k0.hpp it is very close. I do, however, still need to
#define BOOST_NO_LIMITS_COMPILE_TIME_CONSTANTS

That should definitely NOT be needed unless there's something wrong with your numeric_limits specialization?

Then the only open point is:
J 364 ( 35 1 2 )

The result in that case is a denorm at double precision, is that the issue?

@ckormanyos
Copy link
Member Author

ckormanyos commented Dec 19, 2024

What I meant to say was - "I tend to make the same mistake over and over"

Me too.

Ah OK. John with the change in bessel_k0.hpp it is very close. I do, however, still need to
'#define BOOST_NO_LIMITS_COMPILE_TIME_CONSTANTS'

That should definitely NOT be needed unless there's something wrong with your numeric_limits specialization?

Maybe. Or perhaps I don't yet have proper handling of underflow(s) and/or zeros. I'll take a bit more time to figure out what I really need to do. But that is my assignment.

In this PR, there is basically now just one change of an exponent-check constant, two removals of the word constexpr, and the tolerance changes on the new runners. So I'm likely to push it, since there are no controversial changes there. But I believe the backend I'm testing might not quite be right (yet) in some of these edge cases. Let me figure out how to move forward. Then when I have some more well-formulated info, I'll pick up some of the harder points in a newer PR...

@ckormanyos
Copy link
Member Author

OK, so s390 is hanging on drone. Everything else is green. This thing will fix one single failure in the onslaught of cpp_double_fp_backend tests. There will be more work needed.

There is nothing more controversial in this PR. I can play around with it since the erroneous constexpr attributes are corrected.

Also the tolerances are corrected on some of those distributions mentioned above.

So I'll push this thing.

Then I need to study up on the backend details and find out exactly what is causing some of the final edge case failures. So this will be ocntinued in future PRs.

Cc: @jzmaddock and @mborland

@ckormanyos ckormanyos merged commit d026468 into develop Dec 19, 2024
77 of 78 checks passed
@ckormanyos ckormanyos deleted the double_double_batch_01 branch December 19, 2024 20:46
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