SMC and PG algorithms #20
Conversation
Pull Request Test Coverage Report for Build 719106798Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
===========================================
+ Coverage 25.94% 56.19% +30.25%
===========================================
Files 5 5
Lines 424 363 -61
===========================================
+ Hits 110 204 +94
+ Misses 314 159 -155
Continue to review full report at Codecov.
|
I don't have a good enough sense at the moment for some kind of model type. My sense is that it doesn't seem to be needed right now -- adding typing wouldn't help too much. Maybe down the road if
Yeah, that sounds like a bug to me. Including the RNG in the |
There was a problem hiding this comment.
thanks @devmotion, this looks good overall. I left a few minor comments below. The tests look reasonable to me, but I agree we can add a few more tests in future PRs.
| @test getspace(s) === (:x, :y) | ||
| function (m::NormalModel)() | ||
| # First latent variable. | ||
| m.a = a = rand(Normal(4, 5)) |
There was a problem hiding this comment.
Maybe consider passing rng into the model? E.g., function (m::NormalModel; rng = Random.GLOBAL_RNG)() ... end
There was a problem hiding this comment.
I tried this in my local branch when I realized that currently RNGs are not handled corrected by Trace. However, I reverted this change when I realized that this would require significant changes to Trace.
There was a problem hiding this comment.
See also #21 (comment).
There was a problem hiding this comment.
Happy to address this in a separate PR (#23)
test/smc.jl
Outdated
| @test s.resampler == ResampleWithESSThreshold() | ||
| @test getspace(s) === (:x, :y) | ||
| # First observation. | ||
| produce(logpdf(Normal(a, 2), 3)) |
There was a problem hiding this comment.
Maybe consider defining a helper function observe(dist, x) = produce(dist, x)) so that we don't need to explicitly export the produce API from libtask?
|
Tests fail since Libtask_jll is not compatible with Julia 1.6 yet (more information can be found in the discussion in TuringLang/Turing.jl#1554). Maybe we should just run tests with Julia 1.5 until it is fixed? |
Co-authored-by: Hong Ge <hg344@cam.ac.uk>
| @test getspace(s) === (:x, :y) | ||
| function (m::NormalModel)() | ||
| # First latent variable. | ||
| m.a = a = rand(Normal(4, 5)) |
There was a problem hiding this comment.
Happy to address this in a separate PR (#23)
|
Many thanks @devmotion! |
4 participants
This removes Turing-specific parts from the SMC and PG algorithms. The (quite limited) tests copied from Turing pass with a callable model but honestly there might very well be some problems with how these rudimentary models and algorithms handle samples and how they are reset.
Some things I noticed:
sample, hence it might be good to instead implement models asfunction model(rng::AbstractRNG) ... endand to make the RNG explicit inTrace. I assume that any in general it would be easier to replay trajectories if particles/traces would save the unmutated RNG with which the model was called.