Add logit, logistic_sigmoid, and use in logistic dist to support promotion policies
#1297
Conversation
|
Thanks for this Matt! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1297 +/- ##
===========================================
+ Coverage 91.51% 95.07% +3.55%
===========================================
Files 558 796 +238
Lines 53881 67011 +13130
===========================================
+ Hits 49311 63708 +14397
+ Misses 4570 3303 -1267
... and 335 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| if(power < -tools::log_max_value<RealType>()) | ||
| return 1; | ||
| return 1 / (1 + exp(power)); | ||
| RealType power = -(location - x) / scale; |
There was a problem hiding this comment.
Here and below: shouldn't this calculation also use the promoted type? E.g. suppose location is 0, scale is 3, and x is 1. If RealType is double precision, then power is 0.3333333333333333, which becomes 0.33333333333333331483 when promoted to extended precision. If the translation and scaling uses the promoted type, then power should be 0.33333333333333333334, and that is the value that I would expect to be passed to logistic_sigmoid.
There was a problem hiding this comment.
Having power as a promoted type makes sense. I then added the ability to disable further promotion to logistic_sigmoid. I was thinking about this case with the policy from your original issue[1]: RealType is float which means that power would be promoted to double. Without further disabling promotion logistic_sigmoid would compute internally using long double rather than staying at the already promoted type of double.
[1]
typedef policy<
promote_float<true>,
promote_double<true>
> with_promotion;
There was a problem hiding this comment.
Yeah, I was wondering how disabling a second promotion would work out.
Adding that boolean template parameter to the function seems a bit awkward. Now logistic_sigmoid has a regular Policy parameter, and a parameter to disable promotion in that Policy. Most users of logistic_sigmoid would never need it--they could just use an appropriately defined Policy. Also, I think this boolean parameter would have to be added to any other special functions that get called from within distribution functions (assuming an audit of the other distributions finds more cases where promotion isn't fully implemented).
Instead, in the cdf function, could policies::normalise be used to define a new policy from the existing one, with promotion disabled? This appears to be the suggestion given at the bottom of https://www.boost.org/doc/libs/1_87_0/libs/math/doc/html/math_toolkit/pol_tutorial/policy_tut_defaults.html.
So something like
normalise<Policy, promote_float<false>, promote_double<false>>::type
would be the policy used when, say, cdf calls logistic_sigmoid.
This worked for a trivial example that I created with lambert_w0 (just to test the idea), but I haven't tried updating this PR with such a change, so I don't know if there are obstacles to making it work.
Closes: #1294
Closes: #1296