Simplify ParticleContainer and add better tests for propagate! #1242
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1242 +/- ##
=======================================
Coverage 66.25% 66.25%
=======================================
Files 25 25
Lines 1286 1286
=======================================
Hits 852 852
Misses 434 434
Continue to review full report at Codecov.
|
IIRC, these are intended for supporting adaptive resampling, where the particle weights get accumulated for multiple time steps until the ESS falls below some threshold. |
| @test iszero(lz) | ||
| # Create particle container. | ||
| logps = [0.0, -1.0, -2.0] | ||
| particles = [Trace(fpc(logp), empty_model(), sampler, VarInfo()) for logp in logps] |
There was a problem hiding this comment.
This looks good. On a side note, it's strange that we still have the following two Trace constructors:
function Trace(f::Function, m::Model, spl::AbstractSampler, vi::AbstractVarInfo)
function Trace(m::Model, spl::AbstractSampler, vi::AbstractVarInfo)maybe we can consider merging these two in the future.
There was a problem hiding this comment.
Yep, that's a bit strange indeed.
2 participants
This PR simplifies ParticleContainer a bit more by removing the
modelfield which hasn't been used (and was actually misused in the tests by setting it to arbitrary functions). Moreover, it adds the additional tests promised in #1237 (comment).When I added the tests I was a bit surprised actually that the implementation of
propagate!(andconsumebefore, that code was not modified in the recent PR) increases the unnormalized logarithmic weights of the particles with the values that areproduced by the particles and not just sets them to these values (as I would have expected from the algorithm since, if I understand correctly, these values correspond to the log probability of the next "observation"). In the currently implemented samplers (SMCandPG) this does not matter though since the unnormalized weights are reset in theresample!step which is performed before everypropagate!step. Nevertheless, I'm wondering what's the reason for the current implementation?