Hazard specification of parametric baseline hazard #134
Conversation
| // compute discretised hazard | ||
| haz[1] = cdf[1]; | ||
| haz[2:(n-1)] = 1 - ccdf[2:(n-1)]./ccdf[1:(n-2)]; | ||
| haz[n] = 1; |
There was a problem hiding this comment.
Just like in the prob_to_hazard function, we are forced to set haz[n]=1 as otherwise numerical issues can arise (non-finite gradients).
Come to think about it, we are currently not able to actually implement max_strat=0, because we have to set the last hazard to 1 (making it max_strat=1 effectively). So the question is whether we should remove it or just leave it there. It doesn't do much harm at least.
There was a problem hiding this comment.
Yeah this is a good point. It also introduces different behaviour depending on if you use hazards or probability directly. As we don't surface the stan functionality I would suggest we leave in but with a comment noting this issue?
There was a problem hiding this comment.
Hmm perhaps this is related to what I am seeing in #140 with infinite gradients. Not entirely clear if it can be as this is Inf on the logit scale.
There was a problem hiding this comment.
I think it could be the same issue.
Regarding commenting the limitation of max_strat==0 do you want to add something to #140 now that this PR is merged? :)
There was a problem hiding this comment.
Will do - sorry got excited!
| j += is_nan(abs(pmfs[k, i])) ? 1 : 0; | ||
| j += is_inf(abs(pmfs[k, i])) ? 1 : 0; | ||
| j += is_nan(abs(ref_lh[k, i])) ? 1 : 0; | ||
| j += is_inf(abs(ref_lh[k, i])) ? 1 : 0; |
There was a problem hiding this comment.
Replaced pfms with ref_lh. Could be hazards if we want to keep them in transformed parameters. Btw, why do we use abs here (and cmdstan is asking to change it to fabs)?
There was a problem hiding this comment.
In 2.30.0 it warns about fabs. In 2.29.0 it warns about abs. I think they flip-flopped on which to implement. I saw some issues to this effect but now can't find.
There was a problem hiding this comment.
Oh I didn't know they were switching around. My question was more about why we need the absolute here at all, why can't we just plug in the potentially negative values into is_inf? I am probably lacking some knowledge on is_inf here?
There was a problem hiding this comment.
uummmmm I don't know what past Sam was thinking here. Maybe lack support for negatives? I have no idea. Will test.
|
Still need to sort out the tests apparently... |
There was a problem hiding this comment.
Love this so much regardless of speed up! Everything looks good aside from failing tests.
Shame about the direct specification but agree we should focus on keeping the code concise/elegant.
Test failures are a result of changes in what the stan model outputs it looks like. This isn't documented anywhere so my bad.
Essentially need to run tests locally and use testhat::snapshot_review() to check snapshots have changed in the way that is expected (i.e pmfs is gone).
There may be a second test checking the equivalence of the saved example and current output. That needs saved examples to be updated.
Will add making this info public to the improving contributor workflow issue
| // compute discretised hazard | ||
| haz[1] = cdf[1]; | ||
| haz[2:(n-1)] = 1 - ccdf[2:(n-1)]./ccdf[1:(n-2)]; | ||
| haz[n] = 1; |
There was a problem hiding this comment.
Yeah this is a good point. It also introduces different behaviour depending on if you use hazards or probability directly. As we don't surface the stan functionality I would suggest we leave in but with a comment noting this issue?
Codecov Report
@@ Coverage Diff @@
## develop #134 +/- ##
========================================
Coverage 85.12% 85.12%
========================================
Files 12 12
Lines 1190 1190
========================================
Hits 1013 1013
Misses 177 177 Help us with your feedback. Take ten seconds to tell us how you rate us. |
|
I've merged in modular changes from #137 and updated the tests and examples. Seeing a little bit of numerical instability potentially? |
|
Tested this out and no instability. Going to merge but lets keep discussing points above. |
This PR closes #56 by implementing direct computation of hazards for the parametric delay distributions. Only in the case of no reporting effects the probabilities are instead computed (same case distinction as before).
The model fits correctly, but speed is improved only marginally (on the order of 0.1% compared to the likelihood). This may be bigger for larger delays, and anyway I think this is the cooler way to specify the baseline 馃槅
FYI: Initially, I considered adding special cases/shortcuts for the exponential and log-logistic distribution. However, I found that no relevant improvement of speed would be achieved at the cost of making the code less elegant. In particular, the exponential distribution only has a constant hazard if we don't normalize it, so it's not worth implementing a case distinction for this.