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

Replace int with long long in nct pdf and cdf #955

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

mborland
Copy link
Member

See: scipy/scipy#17916

The upper bound for the non-centrality parameter is still lower than 2e16 from the reproducer, but it is much higher than it was. By extension the change also applies to non-central beta distribution.

@jzmaddock
Copy link
Collaborator

Thanks Matt!

@NAThompson
Copy link
Collaborator

FWIW, it wouldn't hurt to git grep for itrunc everywhere and see where it is sensible to replace with lltrunc; I've hit this issue before as well in a different context.

@jzmaddock
Copy link
Collaborator

The failures in this PR should be fixed by boostorg/multiprecision#536. Sorry about that!

@mborland
Copy link
Member Author

mborland commented Feb 22, 2023

@jzmaddock To buy us even more space I think we could conditionally use __int128 instead of long long because k is never streamed. Do you think that would be worthwhile in the event you don't find a better solution in literature?

@jzmaddock
Copy link
Collaborator

To buy us even more space I think we could conditionally use __int128 instead of long long because k is never streamed. Do you think that would be worthwhile in the event you don't find a better solution in literature?

A quick bit of Googling isn't turning much up. And yes I did wonder about using __int128, although we would have to check that a) it's available, and b) that the FP type has an explicit conversion to __int128. I also wonder if we might be into a territory where we're calculating garbage given that ultimately we will have many __int128's mapping to single FP values once they get passed back to the incomplete beta?

The other suggestion was to raise a domain_error in this case, and given that we know the limit on the non-centrality parameter this could be done early on in the distribution constructor?

@mborland
Copy link
Member Author

mborland commented Feb 22, 2023

The other suggestion was to raise a domain_error in this case, and given that we know the limit on the non-centrality parameter this could be done early on in the distribution constructor?

For all of these it would be easy to wrap the call to lltrunc in a try block and then throw domain error in the event it fails.

@jzmaddock
Copy link
Collaborator

Yup, although it circumvents the policy mechanism and building in exception-free environments, so maybe updating the checks for example here:

might be better?

@mborland
Copy link
Member Author

Yup, although it circumvents the policy mechanism and building in exception-free environments, so maybe updating the checks for example here:

might be better?

Do we know the exact values for the nct failures because they are near, but not at the square? The other ones are straight-forward.

@jzmaddock
Copy link
Collaborator

Do we know the exact values for the nct failures because they are near, but not at the square? The other ones are straight-forward.

Ah, good point. Yes we need to be able to square the non-centrality, but in all cases we need to leave a bit of headroom as well, because we need to be able to iterate forward from what is only a starting point. So would sqrt(max) - max_series_iteations be reasonable?

@mborland
Copy link
Member Author

Ah, good point. Yes we need to be able to square the non-centrality, but in all cases we need to leave a bit of headroom as well, because we need to be able to iterate forward from what is only a starting point. So would sqrt(max) - max_series_iteations be reasonable?

I think that would work. Do you me to merge this so you can take over the error handling? I am very bad at working with the policies.

@jzmaddock
Copy link
Collaborator

That's fine yes: BTW I'm still waiting for CI on multiprecision to cycle to fix those last few failures.

Oh, and there's a boost release incoming.... once we're all green again we should probably do a merge to master and then get back to the bugs.

@mborland
Copy link
Member Author

Ok. Once I see the notification that you merged the multiprecision PR I will cycle the CI on this one to confirm the changes.

@jzmaddock
Copy link
Collaborator

Cycling now...

@mborland
Copy link
Member Author

This is back to green so looks like your multiprecision fixes worked. Merging.

@mborland mborland merged commit 5576533 into boostorg:develop Feb 23, 2023
@mborland mborland deleted the 17916_2 branch February 23, 2023 16:21
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