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 gamma_inc for larga a and small x (and very large x) #314

Merged
merged 10 commits into from
May 12, 2021

Conversation

andreasnoack
Copy link
Member

Fixes #311 as explained in my comment there.

@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #314 (ef42fcc) into master (2e90d1b) will increase coverage by 0.35%.
The diff coverage is 93.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
+ Coverage   88.44%   88.79%   +0.35%     
==========================================
  Files          12       12              
  Lines        2622     2624       +2     
==========================================
+ Hits         2319     2330      +11     
+ Misses        303      294       -9     
Flag Coverage Δ
unittests 88.79% <93.13%> (+0.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/gamma_inc.jl 90.22% <93.13%> (+1.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e90d1b...ef42fcc. Read the comment docs.

@andreasnoack
Copy link
Member Author

andreasnoack commented May 11, 2021

I've added some tests to increase the test coverage. While doing that I noticed a bug in rgammax which I've fixed. I'm now folloing the double precision instead the single precision in NSWC. @devmotion Would you be able to review this one? (It will help us towards getting rid of Rmath in StatsFuns).

Update: I've also renamed the stirling function to stirling_error since it returns the difference between the Stirling approximation of loggamma and loggamma so I think it's confusing to call it stirling although that is what's called in IncgamFI module that the some of the implementations are following.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

I did not check the correctness of the parts where only the formatting was changed. I am sure that rgammax matches the Fortran implementation and that _gamma_inc_choose_algorithm also corresponds to the Fortran code (just had a question about the assert statement). It was a bit tricky to follow the Fortran code for _gamma_inc with all the go-to statements. I couldn't find one of the branches in the Fortran code but otherwise it seems to be correct, in particular the additional branch for _gamma_inc_choose_algorithm if a >= big1[iop] and !(abs(s) <= 0.4).

src/gamma_inc.jl Outdated Show resolved Hide resolved
src/gamma_inc.jl Outdated Show resolved Hide resolved
src/gamma_inc.jl Outdated Show resolved Hide resolved
test/beta_inc.jl Outdated Show resolved Hide resolved
test/beta_inc.jl Outdated Show resolved Hide resolved
src/gamma_inc.jl Show resolved Hide resolved
src/gamma_inc.jl Show resolved Hide resolved
end

y = a*z
rta = sqrt(a)
if abs(s) <= e0[iop]/rta
Copy link
Member

Choose a reason for hiding this comment

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

In the Fortran code (end of block 20) I can only find a check for abs(s) <= 0.4 which appears after this branch here in SpecialFunctions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just looked and it seems to be part of the single precision version GRATIO. It's also block 20 and line 13303. However, we shouldn't be using the single precision version (although it seems that they had larger words back when Morris wrote all this code). I'll update this to follow the double precision routine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think it's better to leave that for a separate PR. The single precision version has the extra IOP argument that controls the precision and we expose that in gamma_inc. As mentioned, the word size was larger on the machines that Morris wrote this for and he writes in the comment to the single precision version GRATIO that

IF IND = 0 THEN THE USER IS REQUESTING AS MUCH ACCURACY AS POSSIBLE (UP TO 14 SIGNIFICANT DIGITS).

so I think it's still okay for Float64. Notice that his double precision literals have 30 digits.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this seems reasonable. Thanks for looking into this, I didn't check the single precision version.

end
p = erf(sqrt(x))
return ( p , 1.0 - p )
return (p, 1.0 - p)
elseif x < 1.1
Copy link
Member

Choose a reason for hiding this comment

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

It seems in the Fortran code the cutoff is 2 instead of 1.1 (in the section "SELECT THE APPROPRIATE ALGORITHM" above 10).

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed this is similar to the one above. I'll go through the while function.

src/gamma_inc.jl Outdated Show resolved Hide resolved
version from NSWC. Remove redundant constant. Some style consistency
adjustments.
@andreasnoack
Copy link
Member Author

I think this one is ready.

@devmotion
Copy link
Member

Did you see my suggestions in #314 (comment) and #314 (comment) and similar lines? It seems they were marked as resolved but maybe that happened automatically when you force pushed the branch.

andreasnoack and others added 8 commits May 11, 2021 22:10
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@andreasnoack
Copy link
Member Author

I could have sworn that I'd already accepted all of them. Maybe it's a problem if you don't allow enough time between acceptance of different suggestions.

@andreasnoack andreasnoack merged commit cd39372 into master May 12, 2021
@andreasnoack andreasnoack deleted the an/gamma_inc branch May 12, 2021 06:41
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.

endless loop in gamma_inc?
2 participants