Fix lower incomplete gamma functions with x = 0 #1251
Conversation
In this case, the errno error handling did not work correctly, as internal functions where accidently setting it, although no overflow happens. Fixes boostorg#1249.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1251 +/- ##
===========================================
- Coverage 93.83% 93.82% -0.01%
===========================================
Files 657 658 +1
Lines 55244 55330 +86
===========================================
+ Hits 51840 51916 +76
- Misses 3404 3414 +10
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| @@ -1627,7 +1631,7 @@ BOOST_MATH_GPU_ENABLED T gamma_incomplete_imp_final(T a, T x, bool normalised, b | |||
| #endif | |||
| result = gam - result; | |||
| } | |||
| if(p_derivative) | |||
| if(p_derivative && x > 0) | |||
There was a problem hiding this comment.
I think we need to let this line execute without the check for x > 0, that way our root finder will get a derivative back: in this particular case, any arbitrary large value will do.... ah but there should be an else before the *p_derivative /= x;.
There was a problem hiding this comment.
Hi, I adjusted the coding. Is it okay now?
I also corrected some wrong ternary operators in the file which prevent using some functions with non-standard float/double T template parameters.
|
Other than my one small comment, this all looks good. Many thanks for this! |
|
Looks great, thanks, just running CI now... |
|
Where are these CI errors come from? I don't get them ... |
The Cauchy one was failing with the SYCL compiler, and the beta ones are new failure there that are unrelated. The last one is we need to increment the current version in Boost.Config. I would not worry about any of these failures. I need to address them, and make sure we haven't broken CUDA along the way |
|
Ah shucks, sorry, I hadn't spotted that the new beta tests were failing on CUDA. @mborland some of them are trivial failures (need to update expected error rates), but there are a few "gross" errors which might warrant investigation. Or it may be that the inputs are so extreme that there's not much we can do on that platform. If you have CUDA/Sycl set up can I let you investigate? |
Yes, next week I'll look into these. The Scipy guys also are in a position now to use the CUDA stuff so I'll put some effort into the rest of the library too. |
|
@cohomology can you please merge develop into this PR so we kick off a fresh CI run and do a final check before merging? Thanks! |
|
I don't understand your changes in gamma.hpp. The assert: (x >= 1) || (tools::max_value() * x >= *p_derivative) is true in my case, because in "tools::max_value() * x >= *p_derivative" the left side is 0 and the right side is zero, too. So the division by 0: *p_derivative /= x; gives NaN for the derivate. Is that okay? Or introduce another if? |
|
Ok, adjusted to give the result max/2 as before. |
You're correct: I know I've only just merged these but they're old changes from about a year ago I was half way through and it looks like I missed a case. It can overflow, but only for x denormal or zero. Test case is for a=0.01, x = denom_min. Feel free to reinstate the old commented out code, and I'll try and figure out a way to get coverage later... |
|
Ok, done. CI must still run through, but already was okay with yesterdays commit, except the sycl errors. |
|
Thanks @cohomology . I was about to merge this and then realized that the new test is generating UBSAN failures in cpp_int's internal logic. It would seem to be unrelated to this patch, but I'm wary of introducing a new "known failure". I've tried and failed to reproduce the issues locally, anyone else? @ckormanyos @mborland ? For the record the failure is: |
|
Update, definitely don't understand this one, we have: And this could (should actually) be marked as |
|
I made some recent changes to I'm not sure if this is real or bogus, but I do see that I made a cast to Also I notice that these left shifts are exceeding the range of 64-bit. So maybe I messed that one up and these tests reveal that mess-up. On the other hand, this might be a new phenomenon. I can investigate this tomorrow in peace. But I'm not sure if this is the right place (in my changes) or if we have discovered a new phenomenon? Cc: @jzmaddock and @mborland and @cohomology |
|
Hi @jzmaddock in a PR in multiprecision, I have reverted the recent changes in As soon as that cycles green (as it is expected to), I will merge it into develop of Multiprecision. Could we then, at that point in time, repeat these UBSAN tests? And then we might be able to find out if the recent changes in Cc: @mborland and @cohomology |
|
Not so easy to re-run the drone tests... ah, maybe closing and re-opening might do it? Also need to wait half a day after multiprecision has been merged for the master project to catch up. Might be easier... to import the failing test into multiprecision on your own fork/branch and test locally that way? That's what I was thinking of, but have run out of time at least for today. That would need replicating the failing runner on Gibhub actions as well. But it might make it easier to rapidly get to the cause (there's only really one test runner that needs running) without changing develop? |
Something like that. I could even spin up a dedicated repo and a simplified CI for this purpose.
|
|
Hmmmm... Thinking out loud. Why don't I see if I can reproduce the UBSAN issues locally? I will report back. |
Yes exactly
Yes.
Even better if you can, I don't have a suitable Linux machine setup at present. I did add a multitude of asserts for all those functions and couldn't detect anything with msvc or mingw though. |
|
Sorry for the noise, apparently closing and re-opening doesn't trigger a drone re-build :( |
|
I think the one failure is a false positive, and if everyone agrees I'll disable that test under I made a cut down test PR here: #1267 which pulls multiprecision from the integration_check_do_not_merge branch. I added asserts to the effected code like this: And I think we were misreading this: it's complaining that the value stored in It would be possible to suppress the error by masking off the operand to keep just the bits we're keeping post-shift, but that's silly and hamstrings the code unnecessarily. So basically, my conclusion, is we should just not running the unsigned-integer sanitizer on this code, as it's much too picky and we already have a USAN run anyway. |
|
Hi John @jzmaddock I've been busy in another office. And I could not actually do anything locally. That all makes sense. I had also wanted to add that I have been running Cc: @mborland |
I would agree with this since shifting bits off the left side of an unsigned integer is well defined (they are discarded). |
In this case, the errno error handling did not work correctly, as internal functions where accidently setting it, although no overflow happens.
Fixes #1249.