Particle sampler implementation #11
Conversation
…name randcat to scheme
devmotion
left a comment
devmotion
left a comment
There was a problem hiding this comment.
Great effort!
Unfortunately, I feel a bit uncomfortable reviewing this PR since I don't believe I'm able to spot any regressions or bugs possibly introduced here. I'm not sure on which state of the Turing particle samplers this PR is based, in particular I'm wondering if all recent bug fixes are included (see, e.g., TuringLang/Turing.jl#1345).
It also seems that the files are not just copied from the Turing repo since the formatting of most docstring seems to be incorrect. This should be fixed.
Project.toml
Outdated
| name = "AdvancedPS" | ||
| uuid = "9c00393c-822d-4781-8df5-4c3c33f9866d" | ||
| authors = ["JS Denain <denain.js@gmail.com>"] | ||
| version = "0.1.0" | ||
|
|
||
| [deps] | ||
| AbstractMCMC = "80f14c24-f653-4e6a-9b94-39d6b0f70001" | ||
| Distributions = "31c24e10-a181-5473-b8eb-7969acd0382f" | ||
| DynamicPPL = "366bfd00-2699-11ea-058f-f148b4cae6d8" | ||
| Libtask = "6f1fad26-d15e-5dc8-ae53-837a1d7b8c9f" | ||
| Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c" | ||
| StatsFuns = "4c63d2b9-4356-54db-8cca-17b64c39e42c" | ||
| Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" | ||
| Turing = "fce5fe82-541a-59a6-adf8-730c64b5f9a0" |
There was a problem hiding this comment.
The Manifest.toml file should be removed and Project.toml should be cleaned: test dependencies have to be moved to a separate section (e.g., this package shouldn't depend on Test or Turing, and probably also not on DynamicPPL), and compatibility bounds have to be specified.
There was a problem hiding this comment.
By 'moved to a separate section', do you mean having an [extras] section in Project.toml and a [target] section containing the line test = [<list of packages>]?
|
|
||
|
|
||
|
|
||
| # AdvancedPS.jl |
There was a problem hiding this comment.
Why are the references removed?
| end | ||
|
|
||
| """ | ||
| reset_logweights!(pc::ParticleContainer) |
There was a problem hiding this comment.
Copying messed up the docstrings.
test/resampling.jl
Outdated
| @test sum(resSystematic .== 2) ≈ (num_samples * 0.4) atol=1e-3*num_samples | ||
| @test sum(resStratified .== 2) ≈ (num_samples * 0.4) atol=1e-3*num_samples | ||
| @test sum(resMultinomial .== 2) ≈ (num_samples * 0.4) atol=1e-2*num_samples | ||
| @test sum(resResidual .== 2) ≈ (num_samples * 0.4) atol=1e-2*num_samples |
There was a problem hiding this comment.
| @test sum(resSystematic .== 2) ≈ (num_samples * 0.4) atol=1e-3*num_samples | |
| @test sum(resStratified .== 2) ≈ (num_samples * 0.4) atol=1e-3*num_samples | |
| @test sum(resMultinomial .== 2) ≈ (num_samples * 0.4) atol=1e-2*num_samples | |
| @test sum(resResidual .== 2) ≈ (num_samples * 0.4) atol=1e-2*num_samples | |
| @test count(==(2), resSystematic) ≈ 0.4 * num_samples 0.4atol=1e-3*num_samples | |
| @test count(==(2), resStratified) ≈ 0.4 * num_samples atol=1e-3*num_samples | |
| @test count(==(2), resMultinomial) ≈ 0.4 * num_samples atol=1e-2*num_samples | |
| @test count(==(2), resResidual) ≈ 0.4 * num_samples atol=1e-2*num_samples |
Why are the different tolerances needed BTW?
|
I share @devmotion's concern that it's hard to review this. Could we basically just copy all the Turing code first in a separate PR, merge that through first, and then use the new master to compare the changes made as part of this PR? |
|
AdvancedPS here is supposed to work with the Turing samplers introduced in this PR. |
|
Yeah, that's fine -- it's just difficult to examine the changes because all the code here is "new" to AdvancedPS.jl, so we can't review changes that might be a bad idea because there's nothing to compare to. |
|
Close in favour of #14 |
4 participants
Library for particle samplers in Turing.