-
Notifications
You must be signed in to change notification settings - Fork 248
Adding logarithm of incomplete gamma function #1338
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
Conversation
|
As noted above, there are still a number of issues with this PR. Here's a list of issues that I've run into:
@jzmaddock or @mborland do you have any suggestions? |
| if (std::real(temp) > 0) | ||
| { | ||
| return x1 + log1p(exp(-temp)); | ||
| return x1 + log(1.0 + exp(-temp)); |
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.
This is a really bad change that will kill precision when temp is small.
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 reverted this for the logaddexp function and created a new function for adding complex numbers. I haven't been able to find an equivalent of log1p that works with complex numbers. Could I just make a new function clog1p which computes the Taylor series of
|
|
||
| template <typename Gen, typename U> | ||
| BOOST_MATH_GPU_ENABLED inline typename detail::fraction_traits<Gen>::result_type continued_fraction_b_impl(Gen& g, const U& factor, boost::math::uintmax_t& max_terms) | ||
| BOOST_MATH_GPU_ENABLED inline typename detail::fraction_traits<Gen>::result_type continued_fraction_b_impl(Gen& g, const U& factor, boost::math::uintmax_t& max_terms, bool logArithmetic) |
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.
Since the log and non-log versions basically have completely different code, I see this as two different functions?
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 separated this into a new function in gamma.hpp. I wasn't sure what the correct implementation should have been.
| // | ||
| template <typename Gen, typename U> | ||
| BOOST_MATH_GPU_ENABLED inline typename detail::fraction_traits<Gen>::result_type continued_fraction_a_impl(Gen& g, const U& factor, boost::math::uintmax_t& max_terms) | ||
| BOOST_MATH_GPU_ENABLED inline typename detail::fraction_traits<Gen>::result_type continued_fraction_a_impl(Gen& g, const U& factor, boost::math::uintmax_t& max_terms, bool logArithmetic=false) |
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.
Again this looks like two different functions to me.
| const Real temp = x1 - x2; | ||
|
|
||
| if (temp > 0) | ||
| if (std::real(temp) > 0) |
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.
The trick here, is to add using std::real above, and then call real(temp), that way ADL can find boost::multiprecision::real in the multiprecision case (likewise for any other type that has their own namespace and is forbidden from putting stuff in std::).
|
Some comments in the code, but to address your other points:
Um, yes, but if it's not delivering that already you may need some deep math knowledge to figure out why not.
Yes, although the file is getting a bit big ;) I have a few concerns about doing this in the complex domain:
But don't let me stop you if you want to smack your head against this particular (and complex!) wall ;) And probably just extending the continued fraction code to the complex domain is a worthy goal in it's own right. |
I think the issue stems from adding/subtracting complex numbers. When returning the log of the incomplete gamma function, I add the prefactor Would it be useful to create a function that converts complex numbers to the principle branch?
Surprisingly, I think working with complex numbers is a lot easier. I tried to revert to only using real numbers but the big issue I ran into was taking the log of negative numbers. There are cases in the continued fraction algorithm where I need to take the log of a negative number. With complex numbers this is easy, but if we only wanted to implement real numbers, this would require catching and changing the algorithm accordingly. I'm not sure if I explained this that well. I agree that the complex case won't be used often (if at all). I wouldn't be opposed to accepting real valued inputs, converting to complex values for the calculations, and returning the real value (the imaginary component is always close to 0). This would make it much easier to test. |
Then the issue is this: the continued fraction is only very slowly convergent over some of the domain (especially for So if it was me.... I would simply take the log of Q(a, x) where the answer is known not to underflow (and we know that function "works"), and where underflow is likely because I had a very quick experiment and came up with this, which needs a lot more testing, but seems to work well: |
I didn't realize the CF converges so slowly. I guess that makes sense since it's a sort of iterative method.
That makes a lot more sense. I should have thought of this sooner. Should I delete this PR and start over with your code above? I'm not sure if there's any applications for log arithmetic continued fractions. |
I love continued fractions, but as with series approximations, they always have zones of good/bad convergence: which is only to be expected as for every CF there is a 1:1 mapping onto an equivalent series.
In that case yes, let's start with a fresh PR as it keeps the version history a bit cleaner? |
I've done some quick tests comparing your function to Mathematica and they match up very well! It was accurate for almost all regimes I tried like
The non-policy version seemed pretty easy. I took a look at other functions in I'm not sure what you mean by "promotion boilerplate" - this is my first contribution to boost. Is this in regards to the return argument type |
|
@JacobHass8 Thanks for your help in enhancing boost on behalf of issues raised in SciPy, much appreciated! I thought I would ask if you would be interested in helping out with #1305. Finishing that off will enable SciPy to drop one of the ancient libraries that are unfortunately still in use, cdflib. It could also likely be easier than this issue since we can basically copy paste the approach from other distributions (it is basically a straight forward usage of a derivative-free rootfinder). |
Sure thing! Where exactly do you think I should start? It looks like there are a number of open issues connected to #1305. |
|
@JacobHass8 Awesome! We need to add inverses to the CDFs of the following distributions in boost (see scipy/scipy#21966): Over at SciPy, I had started working on the inverse of the noncentral F CDF with respect to the noncentrality in scipy/scipy#23308. You could look into implementing this in boost itself using the noncentral chi squared code as a template. It should be very similar for other distributions. In case that you do not have a CUDA capable GPU on your own machine, it should be fine to leave out the GPU specific macro stuff for a first PR. Feel free to ping me again in case of questions. |
First draft of adding logarithm of incomplete gamma function. This still has a number of issues. Closes #1173.