-
Notifications
You must be signed in to change notification settings - Fork 251
Implement the log of the incomplete beta function #1359
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1359 +/- ##
===========================================
- Coverage 95.32% 92.01% -3.32%
===========================================
Files 825 598 -227
Lines 67994 56050 -11944
===========================================
- Hits 64815 51574 -13241
- Misses 3179 4476 +1297
... and 322 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
I'd like to ask to reconsider exposing two separate public functions, once You could have one private function which includes the boolean parameter WDYT? |
| } | ||
| else | ||
| { | ||
| return (invert ? (normalised ? 0 : log(boost::math::beta(a, b, pol))) : -std::numeric_limits<T>::infinity()); |
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.
overflow_error again, and this opens a can of worms because the regular beta can suffer underflow too, so that probably needs to be logarithmed too.
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 noticed there wasn't a lbeta function too. For now, it seems like taking log(beta(a, b)) works pretty well. I can open another PR after to implement lbeta though.
| } | ||
| else | ||
| { | ||
| return invert ? log(y) : log(x); |
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.
Just a heads up that these conditionals can trip up expression-template-enabled multiprecision types. You might be safer with log(invert ? y : x) but we can fix this up later.
| [ run test_gamma_mp.cpp /boost/test//boost_unit_test_framework : : : release <define>TEST=3 [ check-target-builds ../config//is_ci_sanitizer_run "Sanitizer CI run" : <build>no ] : test_gamma_mp_3 ] | ||
| [ run test_hankel.cpp /boost/test//boost_unit_test_framework ] | ||
| [ run test_hermite.cpp test_instances//test_instances pch_light /boost/test//boost_unit_test_framework ] | ||
| [ run test_ibeta.cpp test_instances//test_instances pch_light /boost/test//boost_unit_test_framework ] |
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 should be covered by the following lines - we've deliberately split testing ibeta up into smaller chunks because if we test too many types in one go, we get massive object files which fail on cygwin/mingw and run the system out of memory on other CI machines. The "test everything at once" behaviour that is the default is really just there for the convenience of local testing.
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.
Okay! I'll go ahead and delete this line then. I added it because when I ran ../../../b2 test_ibeta from the test folder, test_ibeta wasn't found. It output a ton of linking errors.
|
|
||
| template <class RT1, class RT2, class RT3, class Policy> | ||
| BOOST_MATH_GPU_ENABLED inline typename tools::promote_args<RT1, RT2, RT3>::type | ||
| log_ibeta(RT1 a, RT2 b, RT3 x, const Policy&) |
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.
We need to think about consistency here, based on gamma_p/gamma_q/gamma we should just add a single "l" prefix to make libeta, though I admit it doesn't read too well! Thoughts anyone?
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'd agree that precedent says we should go with libeta even though it's kind of ugly.
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 could go either way with libeta or log_ibeta. The reason I didn't go with libeta is it looks like a library named eta. Using libeta would be consistent with lgamma which might be preferable.
In regards to gamma_p/gamma_q, the incomplete beta function already has the complement implemented as ibetac. To be consistent with this, I would suggest using libeta and libetac.
That's what we have here, but the log functions are called |
|
This is a good first start, but isn't currently much improvement on calling Take for example all the places where we invoke the series evaluation, for example:
That leads us into several failure modes: Here:
This one is probably ignorable too:
Then we hit a whole world of badness here:
logarithm parameter down to this function too.
This
1 as the multiplier, then take the log and add to the result so far.
But now we have a problem - the caller isn't expecting us to return the log of the result - the best I can think of at present is to add a And then work through the other methods of course... |
I'm not sure I agree with this. There are definitely areas that we can improve typedef double T;
T a = 300.25;
T b = 2.75;
T x = static_cast<T>(1) / 2048;
T val = boost::math::log_ibeta(a, b, x);
T prev = log(boost::math::ibeta(a, b, x));
std::cout << "log_ibeta(" << a << "," << b << ")=" << val << std::endl; // log_ibeta(300.25,2.75)=-2279.78
std::cout << "log(ibeta(" << a << "," << b << "))=" << prev << std::endl; // log(ibeta(300.25,2.75))=-infand typedef double T;
T a = 20;
T b = 40;
T x = 0.9;
T val = boost::math::log_ibeta(a, b, x);
T prev = log(boost::math::ibeta(a, b, x));
std::cout << "log_ibeta(" << a << "," << b << ")=" << val << std::endl; // log_ibeta(20,40)=-1.98955e-26
std::cout << "log(ibeta(" << a << "," << b << "))=" << prev << std::endl; // log(ibeta(300.25,2.75))=0and lastly, typedef double T;
T a = 500;
T b = 500;
T x = 0.9;
T val = boost::math::log_ibeta(a, b, x);
T prev = log(boost::math::ibeta(a, b, x));
std::cout << "log_ibeta(" << a << "," << b << ")=" << val << std::endl; // log_ibeta(500,500)=-2.23212e-224
std::cout << "log(ibeta(" << a << "," << b << "))=" << prev << std::endl; // log(ibeta(300.25,2.75))=0In my testing, Maybe I'm missing something easy though. I'll wait to see what you think before pushing ahead any farther on this PR. |
Ah ok, then nevermind and sorry for the noise! |
|
I think you're code succeeds only because you have internal promotion to long double turn ON, if you turn it off (or indeed build with msvc) then your log_ibeta raises an overflow error from |
See #1173.