Manual type promotion for arcsine distribution#1373
Manual type promotion for arcsine distribution#1373JacobHass8 wants to merge 6 commits intoboostorg:developfrom
Conversation
|
@mborland what do you think of this version? I made a number of actual changes to the code that I'd like to get your opinion on. I left comments below. On an unrelated note, I wasn't able to get |
| if (x == x_min) | ||
| { | ||
| return 1; | ||
| } | ||
| else if (x == x_max) | ||
| { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
These special cases used to be swapped (e.g. when x == x_min we returned 0). Is this because the input to ccdf is actually the complement of x? I was very confused reading this right below the definition of cdf.
There was a problem hiding this comment.
I believe so yes, but this doesn't look right to me, and will surely only work for x_min, x_max = [0,1] ?
There was a problem hiding this comment.
Okay, since we're using acos then x really is the complement 1-x (when arcsine is defined on [0,1]). In this case, I think what I have written is correct. When x==x_min, the ccdf becomes 2/pi acos(0)=1.
Why do you say this should only work for x_min, x_max = [0,1]?
There was a problem hiding this comment.
When dealing with the complement, isn't the range (of the complement that is, and we really should have named the variable something other than x in the code) now [1 - x_max, 1 - x_min] ? Sorry, it's late here, and my brain is frazzled after a long week... so I might be talking rubbish ;)
There was a problem hiding this comment.
No, I think since x is defined on [x_min, x_max] then the complement is also defined on [x_min, x_max].
There was a problem hiding this comment.
Hmmm, if x is in [1,2] then 1-x must be in [-1,0] surely?
There was a problem hiding this comment.
Yes, but why does the complement of x have to be 1-x? Isn't the complement something like 3-x? This maps x=2 to 1 and x=1 to 2.
| result = -3; | ||
| return result / 2; | ||
| typedef policies::evaluation_t<RealType, Policy> policy_promoted_type; | ||
| return static_cast<RealType>(static_cast<policy_promoted_type>(-3) / static_cast<policy_promoted_type>(2)); |
There was a problem hiding this comment.
Can't -1.5 be expressed exactly in binary? If so, then we don't have to go through all this type promotion.
There was a problem hiding this comment.
You should be able to. You could also make this:
static const auto return_val {static_cast<RealType>(static_cast<policy_promoted_type>(-3) / static_cast<policy_promoted_type>(2))};
return return_val;There was a problem hiding this comment.
Yes, and decimal too ;) So I'd say we're covered.
There was a problem hiding this comment.
Wait, so should I replace it with
static const auto return_val {static_cast<RealType>(static_cast<policy_promoted_type>(-3) / static_cast<policy_promoted_type>(2))};
return return_val;or just
return -1.5
It is setup to run, and I just tried it on head and the test ran fine. What's the issue? You can try |
|
I do think this approach looks better. Thanks! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1373 +/- ##
===========================================
- Coverage 95.34% 95.33% -0.01%
===========================================
Files 825 825
Lines 68160 68167 +7
===========================================
+ Hits 64986 64989 +3
- Misses 3174 3178 +4
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There were just a ton of errors in my code. I missed a couple of closing brackets which caused a large amount of errors. |
3 participants
This tries to rewrite #1299 in a more human readable way. See #1294 for the same issue with the logistic distribution.