Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rewrite
gamma_inc_taylor
andgamma_inc_asym
more concisely #443Rewrite
gamma_inc_taylor
andgamma_inc_asym
more concisely #443Changes from 13 commits
eb71c65
2363f8f
0b0a61a
46a7b39
4e2edc6
7ac105e
c095329
c058a2a
ee41ab7
83afebb
34229ef
482516c
8d37a34
36d24be
3e06e43
79bacae
1c41a3d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if summation in reverse order is necessary (something like #442 (comment) would be much simpler implementation-wise)? I wonder if there is any argument/comment regarding this choice in the original Fortran code - does anyone know where the current implementation was copied from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory better for numerical stability and precision to do it this way, because otherwise you can lose the accumulated effect of the small components when they're very much smaller than the large components, but I don't know in practice if/when it affects results of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found a couple possible sources, but I think I'm gathering that the impetus was to support arbitrarily low precision. Maybe with Float64 it works pretty much fine, which this package forces. But with lower precision, maybe the errors would add up too much if floating point math wasn't carefully managed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like @Sumegh-git and @simonbyrne were writing/reviewing the PR that introduced them, if they can comment :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACM TOMS (FREE ACCESS): https://dl.acm.org/doi/abs/10.1145/22721.23109
Fig. 2 in the paper shows the flowchart of the algorithm as a whole.
gamma_inc_taylor
: Equ. 15gamma_inc_asym
: Equ. 16The original paper is mentioned in the code comments.
Perhaps those comments should be moved to the function documentation.
SpecialFunctions.jl/src/gamma_inc.jl
Lines 831 to 834 in fdf95ab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! In that case maybe this is the FORTRAN code itself? https://jblevins.org/mirror/amiller/inc_gam.f90