Deprecate AdvancedPS#2848
Conversation
I fixed this locally, but forgot to commit to the PR Co-authored-by: Penelope Yong <penelopeysm@gmail.com>
Co-authored-by: Penelope Yong <penelopeysm@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2848 +/- ##
==========================================
- Coverage 85.09% 84.05% -1.04%
==========================================
Files 23 24 +1
Lines 1516 1643 +127
==========================================
+ Hits 1290 1381 +91
- Misses 226 262 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
the unit tests currently failing are only off by a slightly larger absolute tolerance. Not totally convinced its the fault of the sampler, but I'd appreciate some feedback/sanity checks |
Co-authored-by: Shravan Goswami <123811742+shravanngoswamii@users.noreply.github.com>
yebai
left a comment
There was a problem hiding this comment.
I did a partial review today. Here are a few comments:
It would be helpful to include a high-level overview of the design of the new algorithm. I realize this is an unusual request for a code review, but particle Gibbs is a subtle algorithm, and it's difficult to assess correctness without understanding the overall design and implementation.
I think it's worth making a strong effort to avoid task-local storage and keep the implementation as functional as possible (i.e., by tracking all state explicitly in the TapedTask or particle).
I understand this is a substantial piece of work, so I'm happy to iterate on this over a few review cycles.
The latest commit updates the interface to use taped globals.
Under the hood,
There are a handful of similarities between my implementation and AdvancedPS. I suggest comparing specifically this AdvancedPS code and the highlighted section of my rewrite; which should provide a bit more clarity in terms of correctness of implementation. To ensure this implementation is indeed valid, I wrote a few unit tests I feel are worth highlighting:
|
|
Many thanks, @charlesknipp -- can you help take a look at #2853? #2853 passes all existing and extra tests and also fixes a few other legacy issues that date back to the early days of Turing.jl. We could try to find a way to consolidate this PR and #2853 in the end. |
To facilitate a more uniform ecosystem, this PR is a means of deprecating AdvancedPS in favor of a Turing specific sequential Monte Carlo interface.
For context, this is a little more than a reorganization.
Design Choices
Libtask.produceno longer occurs in the accumulator to ensure that a varinfo reflects the produces log scoreresampleandvarinfoin the task global storageParticleContainerno longer requires tracked RNG, which should be handled in the samplerAbstractMCMC.sampleinstead of stepping through and bundling with the APIMinor User Facing Changes
Multinomial,Systematic, andESSResampler)Criticisms
DynamicPPLLibtaskExt.jlParticleContaineror keep it as an alias forVector{<:Particle}Feel free to make changes. I am open to suggestions. Otherwise, unit tests pass locally so it should be ready to merge.