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

Fix for scipy issue 18302 #977

Merged
merged 2 commits into from
Apr 21, 2023
Merged

Fix for scipy issue 18302 #977

merged 2 commits into from
Apr 21, 2023

Conversation

mborland
Copy link
Member

See: scipy/scipy#18302

Adds bounds checking so we don't return a pdf larger than beta, and catches an overflow exception when x is near std::numeric_limits::min.

@WarrenWeckesser
Copy link
Contributor

Thanks for the fast response, @mborland!

It looks like the code in test/scipy_issue_18302.cpp uses the default policy--is that correct? I wrote a short C++ code to check the case x = 1e-310, and I didn't get the overflow error with the default policy. I had to use promote_double<false>.

@mborland
Copy link
Member Author

Thanks for the fast response, @mborland!

It looks like the code in test/scipy_issue_18302.cpp uses the default policy--is that correct? I wrote a short C++ code to check the case x = 1e-310, and I didn't get the overflow error with the default policy. I had to use promote_double<false>.

I did get overflow with the default policy. Now that you bring it up I ran it on an ARM Mac which means long double = double. I think in this case it's worth effectively ignoring the overflow exception because we know what the result is supposed to be when that happens.

@WarrenWeckesser
Copy link
Contributor

Ah, I see. I tested on a Linux machine, where long double is extended precision. I was just worried that the test wasn't actually testing the fix because it would pass even without the fix. But that is not the case on platforms where long double is double.

@mborland
Copy link
Member Author

Ah, I see. I tested on a Linux machine, where long double is extended precision. I was just worried that the test wasn't actually testing the fix because it would pass even without the fix. But that is not the case on platforms where long double is double.

We recently added ARM and s390x to the CI system so we have 64, 80, and 128 bit long doubles are all tested now. Its helped find overflows and precision issues.

@WarrenWeckesser
Copy link
Contributor

Nice. I think it might be slightly better to use promote_double<false> so the test in fact fails on all platforms without the fix, and therefore the test is verifying that the change actually fixes the problem on all platforms, but obviously that's your call.

@jzmaddock
Copy link
Collaborator

@mborland I think we should fix this updatream in ibeta_derivative here:

T f1 = ibeta_power_terms<T>(a, b, x, 1 - x, lanczos_type(), true, pol, 1 / y, function);
It's the 1/y that fails and we need to decide whether to return the derivative at zero (which may well be infinite) or at tools::min_value<T>() which may well not be. I'm edging towards the latter as the easier fix.

@mborland
Copy link
Member Author

@jzmaddock This is green and I think hits your comments from both threads.

@mborland
Copy link
Member Author

@jzmaddock Any other changes for this one, or good to merge?

@jzmaddock
Copy link
Collaborator

Looks good to me!

@mborland mborland merged commit 298a243 into develop Apr 21, 2023
@mborland mborland deleted the 18302 branch April 21, 2023 10:31
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