add acceptance rate info to Transition struct - #88
Conversation
devmotion
left a comment
devmotion
left a comment
There was a problem hiding this comment.
Could this break Turing? IIRC at some point (and maybe still?) we constructed Transition objects there.
|
This latest iteration pushes |
|
Any thoughts on this since the last changes? I need to go through Turing to find the small adjustments that will be needed to work with this but I want to get any feedback on this implementation before I do. |
| proposal::D | ||
|
|
||
| MALA{D}(proposal::D) where {D} = new{D}(proposal) | ||
| accepted_draws :: Int |
There was a problem hiding this comment.
I don't remember the previous discussion but this design seems a bit surprising to me. I think the number of accepted and total draws is not a property of the sampler but rather of the resulting chain.
There was a problem hiding this comment.
Yeah, the thought here was that samplers have a record of success or failure. This would be useful for implementing delayed rejection or other sample massaging. But that information could probably be retrieved from the vector of transitions along the way.
With that in mind, maybe we can stick with Transition objects just storing whether the sample was accepted or rejected and compute final stats in chain construction or in sampling if needed. I know this PR is already a mess, but I'll revert and make the adjustments.
|
Is there any plan to push this forward? |
|
I personally see no issue with this. If tests pass I say we merge it in. |
|
Great! My local tests are passing. Any idea why the nightly tests are failing in the CI? |
|
I'm going to merge this in for now. Test failures are all on nightly. As a side note, we should maybe consider not testing on nightly, or at least permitting failure. |
|
Tbh I'm not terribly concerned with nightly at the moment. Thank you @cnrrobertson for the PR! Sorry it took so long to merge. I'll increment the patch number separately and flag a release. |
|
My impression the last time I checked the PR was that AbstractMCMC features such as |
5 participants
Solves (at least partially) #40.