Fix biased PG/CSMC posteriors: make delete_retained! mutate - #2855
Merged
Conversation
`PG`/`CSMC` posteriors were biased. `AdvancedPS.fork` marks a fork of the reference as no longer retained by calling `delete_retained!` for its side effect and discarding the return value, but our hook returned a fresh `TracedModel` instead of mutating, so `resample` stayed `false`. Every descendant of the reference therefore kept replaying the retained values in `tilde_assume!!` -- it was a copy of the reference, not a branch off it -- and the sweep lost the diversity that makes particle Gibbs valid. On a two-state HMM with ten observations, `PG(16)` state marginals were up to seven Monte Carlo standard errors from the exact forward-backward values, and are now within one. Present since v0.41.0. Co-Authored-By: Claude Code <noreply@anthropic.com>
Contributor
|
Turing.jl documentation for PR #2855 is available at: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2855 +/- ##
=======================================
Coverage 85.09% 85.10%
=======================================
Files 23 23
Lines 1516 1517 +1
=======================================
+ Hits 1290 1291 +1
Misses 226 226 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The previous test asserted on the `resample` flag through `AdvancedPS.fork`, which only a trace-based implementation has. This one goes through `sample` only: a Gibbs run over a switching model, compared against the exact posterior from enumerating all `2 * 2^8` configurations weighted by the model's own log density. It is also sensitive to the other way a reference can be wrong. Rebuilding it by replaying random numbers rather than reusing values drifts off the retained trajectory once the other Gibbs component re-conditions the latents, which needs the chain structure and the parameter placement this model has. Mean absolute error over the state marginals, across four seeds: 0.002 to 0.005 for a correct sweep, 0.020 to 0.028 for descendants that copy the reference, 0.017 to 0.022 for a replayed reference. The threshold is 0.01, and the testset costs about 50 seconds. Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
yebai
commented
Aug 19, 2026
| return TracedModel(trace.model, trace.varinfo, true, trace.fargs, trace.kwargs) | ||
| # reference particle but rather sample new values. This has to mutate: | ||
| # `AdvancedPS.fork` calls it for its side effect and discards the return value. | ||
| trace.resample = true |
Member
Author
There was a problem hiding this comment.
This is the fix.
The AdvancedPS sampler is correct. The bug was likely introduced due to inconsistent interface changes between AdvancedPS and Turing.jl.
The comment now states the property the test rests on -- conditional SMC is invariant only if the reference is exactly the retained path -- and the replay rule that makes the parameter's placement in the transition a requirement rather than a choice. In the model, the stay probability becomes a transition matrix indexed by the previous state, replacing a ternary over two probability vectors. The exact posterior now sums the parameter out in its own step, and reads the marginals off a flat vector of paths instead of indexing into tuples. Same numbers: the unfixed sampler still gives 0.0354. Co-Authored-By: Claude Code <noreply@anthropic.com>
sunxd3
approved these changes
Aug 21, 2026
sunxd3
left a comment
Member
There was a problem hiding this comment.
really subtle, thanks for the debug and fix!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
PG/CSMCposteriors have been biased since v0.41.0.AdvancedPS.forkmarks a fork of the reference particle as no longer retained by callingdelete_retained!for its side effect and discarding the return value. Turing's AdvancedPS integration returned a freshTracedModelrather than mutating the one it was handed, soresamplenever becametrue.That flag is what
tilde_assume!!consults. With it stillfalse, a descendant of the reference found every address present in its copy of the retained varinfo and took theInitFromParamsbranch, replaying the retained values instead of drawing new ones. Every offspring of the reference was therefore a duplicate of it rather than a branch off it, the population lost the diversity conditional SMC depends on, and the retained trajectory was over-selected.The error is minor but statistically detectable. On a two-state HMM with ten observations,
PG(16)state marginals over 20k draws sat up to 7.0 batch-means standard errors from the exact forward–backward values, and 1.3 after the fix. UnderGibbs(:p => MH(), :z => CSMC(16)), withpthe transition probability, 6.4 and 7.4 standard errors on two seeds, and 1.7 after.The regression test
This PR adds a targeted regression test. The test reaches the sampler only through$T = 8$ whose transition parameter is a second unknown:
sample, so it applies to any PG/CSMC implementation, not to this one's internals. The model is a two-state hidden Markov chain of lengthfor$t = 2, \dots, T$ , where $P_{k,k} = p$ and $P_{k,3-k} = 1 - p$ , so the chain stays put with probability $p = (0.35, 0.65)_i$ . The emission means are $\mu = (-1, +1)$ with $\sigma = 0.8$ . Sampled with
Gibbs(@varname(i) => MH(), @varname(z) => CSMC(8)).Why this model
Conditional SMC leaves$p(z \mid i, y)$ invariant only if the reference particle is exactly the retained path $z^{(n)}$ . There are two ways to lose that, and each ingredient of the model is there to expose one of them.
Descendants that copy the reference. After resampling, a child assigned the reference as its ancestor must continue with fresh randomness; if it keeps replaying the retained values it is the reference again, so the population over-counts one path and the weights stop representing the filtering distribution. Detecting this needs uneven weights, hence means separated by$2$ at $\sigma = 0.8$ : putting a state in the wrong place costs $\approx (2/\sigma)^2/2 \approx 3$ nats, so the ESS gate triggers resampling at almost every step.
A reference rebuilt by replaying uniforms. Replay regenerates the path by pushing the stored uniforms back through the sampling map$z_t = g(u_t; \theta, z_{t-1})$ , where $g$ is the inverse CDF of the latent's conditional — here $z_t = z_{t-1}$ iff $u_t < p$ . That is faithful only while $p$ is fixed: once the MH step moves $i$ , every $u_t$ between $p$ and $p'$ flips its step, and because $z_{t-1}$ is itself an argument of $g$ , one flip changes what "stay" means for the rest of the path. Two design consequences follow, in the notes below.
Exact target
The configuration space is finite,$2 \cdot 2^{8} = 512$ points, so the posterior is available in closed form — weighted by the model's own log density $\ell$ , not by a reimplementation of it:
and the state marginal it implies is
The statistic over 6000 draws is the mean absolute error of the state marginals:
Averaging over$t$ rather than taking a maximum is deliberate: the Monte Carlo error at each $t$ has random sign and partly cancels, while both defects shift many marginals in the same direction and add up.
Result
The same test body, three seeds, four implementations:
Twelve runs, no misclassification. The worst passing value leaves 1.9x headroom under the threshold and the weakest bias signal sits 2.2x above it, with a factor of four between the two groups and nothing in between. The testset costs about 50 s.
That #2853 passes on the same tolerance is a check on the test rather than on that branch: it shares no code with the AdvancedPS-based implementation, so anything the test measures is a property of the sampler's output distribution.
Design notes on the regression test
The parameter assigned to the$g$ involves only the parameters of the conditional being sampled. An emission parameter such as $\sigma$ appears in $p(y_t \mid z_t)$ , a density that is evaluated and never sampled, so $g$ never sees it, and replay reproduces the retained path exactly — the reference comes out right by accident. Measured: with the parameter moved to the emission, the error on #2848 falls from 0.017–0.022 to 0.0028, which is noise.
MHGibbs step has to be the transition. The mapThe latents have to be dependent. With independent latents, the target factorises,$p(z \mid \theta, y) = \prod_t p(z_t \mid \theta, y_t)$ , and the reference has no lineage to corrupt: its value at step $t$ enters only its own weight at step $t$ . A reference regenerated under $p'$ is then one more prior-like draw among the $N - 1$ fresh ones, exchangeable with them, and the population is a plain importance sampler for the right target. In the Markov chain, the reference is a lineage — its descendants inherit the corrupted prefix at every resampling step — so the error compounds along $t$ and again across outer Gibbs iterations, which shows up as a shift in path functionals: the expected number of switches moves by $+0.06$ to $+0.14$ .