Feature/issue 333 fix gamma q chain

[syclik]

Summary: What does this pull request do in general terms?

Fixes #333. Prevents an infinite loop in the derivative of the gamma_q function by throwing an exception. Removed duplicated code in gamma_q; calling function in src/stan/prob/internal_math.hpp instead.

Intended Effect: What is the expected effect of the pull request? Specifically, indicate how the behavior will be different than before the pull request.

Instead of getting stuck in an infinite loop, throws a std::domain_error. This is done in internal_math.hpp, in the stan::math::gradRegIncGamma function.

How to Verify: How can reviewers verify the request has the intended effect? This should include descriptions of any new tests that are added or old tests that are updated. It should also indicate how the code was tested.

Reviewer can run two tests:

• make test/agrad/rev/gamma_q
• make test/math/prob/internal_math

2 additional tests in src/test/agrad/rev/gamma_q_test.cpp that triggered the infinite loop before the fix:

• one for (var,var)
• one for (var, double)
  2 new tests in src/test/agrad/rev/internal_math_test.cpp
• one that should normal execution
• one that shows triggered the infinite loop before the fix

Side Effects: Does the pull request contain any side effects?

No.

Documentation: If the pull request is user facing, how is it documented? Are there examples of how to use the new behavior that users need to know about?

N/A. Internal math function.

Reviewer Suggestions: Who should look at the pull request for code review?

@bob-carpenter: a question about efficiency. Is the check too expensive?

[stan-buildbot]

Test FAILed.
Refer to this link for build results: http://d1m1s1b1.stat.columbia.edu:8080/job/Stan%20Github%20Pull%20Requests/293/

[syclik]

Jenkins, retest this please.

[betanalpha]

Just an implementation note -- I was playing around with this over the weekend
and the nominal analytic continuation doesn't seem to be differentiable. There
is another continuation that might work (z -> 1 / z) but I'd need to play around
with Mathematica to figure out how to implement the resulting Meijer G-function.

On Nov 27, 2013, at 2:41 AM, Daniel Lee notifications@github.com wrote:

• Summary: What does this pull request do in general terms?
Fixes #333. Prevents an infinite loop in the derivative of the gamma_q function by throwing an exception. Removed duplicated code in gamma_q; calling function in src/stan/prob/internal_math.hpp instead.

• Intended Effect: What is the expected effect of the pull request? Specifically, indicate how the behavior will be different than before the pull request.
Instead of getting stuck in an infinite loop, throws a std::domain_error. This is done in internal_math.hpp, in the stan::math::gradRegIncGamma function.

• How to Verify: How can reviewers verify the request has the intended effect? This should include descriptions of any new tests that are added or old tests that are updated. It should also indicate how the code was tested.
Reviewer can run two tests:

• make test/agrad/rev/gamma_q
• make test/math/prob/internal_math
2 additional tests in src/test/agrad/rev/gamma_q_test.cpp that triggered the infinite loop before the fix:

• one for (var,var)
• one for (var, double) 2 new tests in src/test/agrad/rev/internal_math_test.cpp
• one that should normal execution
• one that shows triggered the infinite loop before the fix

• Side Effects: Does the pull request contain any side effects?

No.

• Documentation: If the pull request is user facing, how is it documented? Are there examples of how to use the new behavior that users need to know about?
N/A. Internal math function.

• Reviewer Suggestions: Who should look at the pull request for code review?
@bob-carpenter: a question about efficiency. Is the check too expensive?

You can merge this Pull Request by running

git pull https://github.com/stan-dev/stan feature/issue-333-fix-gamma_q-chain
Or view, comment on, or merge it at:

#418

Commit Summary

• added two tests that fail in gamma_q chain calculations
• refactored code. The same implementation was in src/stan/prob/internal_math.hpp. This doesn't solve the infinite loop problem.
• updating stan/prob/internal_math function to throw an exception when the sum diverges
File Changes

• M src/stan/agrad/rev/gamma_q.hpp (58)
• M src/stan/prob/internal_math.hpp (54)
• M src/test/agrad/rev/gamma_q_test.cpp (20)
• A src/test/prob/internal_math_test.cpp (21)
Patch Links:

• https://github.com/stan-dev/stan/pull/418.patch
• https://github.com/stan-dev/stan/pull/418.diff

[stan-buildbot]

Test FAILed.
Refer to this link for build results: http://d1m1s1b1.stat.columbia.edu:8080/job/Stan%20Github%20Pull%20Requests/294/

[bob-carpenter]

I'm beginning to understand what a heroic effort
it was to implement these in the first place!

I'm OK with just constraining the domains until we can
find better implementations. Not ideal, but better than
an infinite loop.

• Bob

On 11/27/13, 4:07 AM, Michael Betancourt wrote:

Just an implementation note -- I was playing around with this over the weekend
and the nominal analytic continuation doesn't seem to be differentiable. There
is another continuation that might work (z -> 1 / z) but I'd need to play around
with Mathematica to figure out how to implement the resulting Meijer G-function.

On Nov 27, 2013, at 2:41 AM, Daniel Lee notifications@github.com wrote:

• Summary: What does this pull request do in general terms?
Fixes #333. Prevents an infinite loop in the derivative of the gamma_q function by throwing an exception. Removed
duplicated code in gamma_q; calling function in src/stan/prob/internal_math.hpp instead.

• Intended Effect: What is the expected effect of the pull request? Specifically, indicate how the behavior will be
different than before the pull request.
Instead of getting stuck in an infinite loop, throws a std::domain_error. This is done in internal_math.hpp, in the
stan::math::gradRegIncGamma function.

• How to Verify: How can reviewers verify the request has the intended effect? This should include descriptions of
any new tests that are added or old tests that are updated. It should also indicate how the code was tested.
Reviewer can run two tests:

• make test/agrad/rev/gamma_q
• make test/math/prob/internal_math
2 additional tests in src/test/agrad/rev/gamma_q_test.cpp that triggered the infinite loop before the fix:

• one for (var,var)
• one for (var, double) 2 new tests in src/test/agrad/rev/internal_math_test.cpp
• one that should normal execution
• one that shows triggered the infinite loop before the fix

• Side Effects: Does the pull request contain any side effects?

No.

• Documentation: If the pull request is user facing, how is it documented? Are there examples of how to use the new
behavior that users need to know about?
N/A. Internal math function.

• Reviewer Suggestions: Who should look at the pull request for code review?
@bob-carpenter: a question about efficiency. Is the check too expensive?

You can merge this Pull Request by running

git pull https://github.com/stan-dev/stan feature/issue-333-fix-gamma_q-chain
Or view, comment on, or merge it at:

#418

Commit Summary

• added two tests that fail in gamma_q chain calculations
• refactored code. The same implementation was in src/stan/prob/internal_math.hpp. This doesn't solve the infinite
loop problem.
• updating stan/prob/internal_math function to throw an exception when the sum diverges
File Changes

• M src/stan/agrad/rev/gamma_q.hpp (58)
• M src/stan/prob/internal_math.hpp (54)
• M src/test/agrad/rev/gamma_q_test.cpp (20)
• A src/test/prob/internal_math_test.cpp (21)
Patch Links:

• https://github.com/stan-dev/stan/pull/418.patch
• https://github.com/stan-dev/stan/pull/418.diff

—
Reply to this email directly or view it on GitHub #418 (comment).

[syclik]

It's a good thing we have so many tests in place. The chi-square
distribution is now failing while calculating the cdf. I guess the check
was too aggressive.

I'll resubmit when I have a fix.

On Wed, Nov 27, 2013 at 11:50 AM, Bob Carpenter notifications@github.comwrote:

I'm beginning to understand what a heroic effort
it was to implement these in the first place!

I'm OK with just constraining the domains until we can
find better implementations. Not ideal, but better than
an infinite loop.

• Bob

On 11/27/13, 4:07 AM, Michael Betancourt wrote:

Just an implementation note -- I was playing around with this over the
weekend
and the nominal analytic continuation doesn't seem to be differentiable.
There
is another continuation that might work (z -> 1 / z) but I'd need to
play around
with Mathematica to figure out how to implement the resulting Meijer
G-function.

On Nov 27, 2013, at 2:41 AM, Daniel Lee notifications@github.com
wrote:

• Summary: What does this pull request do in general terms?
Fixes #333. Prevents an infinite loop in the derivative of the gamma_q
function by throwing an exception. Removed
duplicated code in gamma_q; calling function in
src/stan/prob/internal_math.hpp instead.

• Intended Effect: What is the expected effect of the pull request?
Specifically, indicate how the behavior will be
different than before the pull request.
Instead of getting stuck in an infinite loop, throws a
std::domain_error. This is done in internal_math.hpp, in the
stan::math::gradRegIncGamma function.

• How to Verify: How can reviewers verify the request has the intended
effect? This should include descriptions of
any new tests that are added or old tests that are updated. It should
also indicate how the code was tested.
Reviewer can run two tests:

• make test/agrad/rev/gamma_q
• make test/math/prob/internal_math
2 additional tests in src/test/agrad/rev/gamma_q_test.cpp that
triggered the infinite loop before the fix:

• one for (var,var)
• one for (var, double) 2 new tests in
src/test/agrad/rev/internal_math_test.cpp
• one that should normal execution
• one that shows triggered the infinite loop before the fix

• Side Effects: Does the pull request contain any side effects?

No.

• Documentation: If the pull request is user facing, how is it
documented? Are there examples of how to use the new
behavior that users need to know about?
N/A. Internal math function.

• Reviewer Suggestions: Who should look at the pull request for code
review?
@bob-carpenter: a question about efficiency. Is the check too
expensive?

You can merge this Pull Request by running

git pull https://github.com/stan-dev/stanfeature/issue-333-fix-gamma_q-chain
Or view, comment on, or merge it at:

#418

Commit Summary

• added two tests that fail in gamma_q chain calculations
• refactored code. The same implementation was in
src/stan/prob/internal_math.hpp. This doesn't solve the infinite
loop problem.
• updating stan/prob/internal_math function to throw an exception when
the sum diverges
File Changes

• M src/stan/agrad/rev/gamma_q.hpp (58)
• M src/stan/prob/internal_math.hpp (54)
• M src/test/agrad/rev/gamma_q_test.cpp (20)
• A src/test/prob/internal_math_test.cpp (21)
Patch Links:

• https://github.com/stan-dev/stan/pull/418.patch
• https://github.com/stan-dev/stan/pull/418.diff

—
Reply to this email directly or view it on GitHub <
https://github.com/stan-dev/stan/pull/418#issuecomment-29369328>.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/418#issuecomment-29400055
.

[stan-buildbot]

Test PASSed.
Refer to this link for build results: http://d1m1s1b1.stat.columbia.edu:8080/job/Stan%20Github%20Pull%20Requests/295/