Skip to content

DPPL 0.37 compat for particle MCMC #2625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 31, 2025

Conversation

mhauru
Copy link
Member

@mhauru mhauru commented Jul 22, 2025

This may or may not be done functionality-wise, except VariableOrderAccumulator seems to go wrong when using Gibbs. (This is probably the same problem as before in merging accumulators.) Let's see how CI does. The code has some pretty ugly bits though, that should be improved.

FYI @penelopeysm

Copy link
Contributor

Turing.jl documentation for PR #2625 is available at:
https://TuringLang.github.io/Turing.jl/previews/PR2625/

Copy link

codecov bot commented Jul 22, 2025

Codecov Report

❌ Patch coverage is 6.66667% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.49%. Comparing base (7ca59ce) to head (6d6fac8).
⚠️ Report is 2 commits behind head on mhauru/dppl-0.37.

Files with missing lines Patch % Lines
src/mcmc/particle_mcmc.jl 0.00% 54 Missing ⚠️
src/mcmc/ess.jl 0.00% 2 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##           mhauru/dppl-0.37    #2625      +/-   ##
====================================================
- Coverage             24.84%   24.49%   -0.35%     
====================================================
  Files                    22       22              
  Lines                  1469     1498      +29     
====================================================
+ Hits                    365      367       +2     
- Misses                 1104     1131      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@penelopeysm
Copy link
Member

As discussed earlier it's looking good. Finer details we talked about:

  • Having a specific accumulator for the particle weight (i.e., the log-likelihood of the most recent observation). Adding a quantity to this accumulator would be responsible for triggering the resampling (via Libtask.produce).
  • This would allow us to accumulate logprior / loglikelihood ( / logjac) as usual, so that they don't have a special meaning inside the pMCMC file (the current code hijacks vi.logp[] for its own purposes).
  • Unsure whether @addlogprob! should also trigger resampling. We think it probably should (if it is incrementing the likelihood by a nonzero amount).
  • We'll probably leave set_retained_vns_del! there for now.

@penelopeysm
Copy link
Member

score = consume(trace.model.ctask)
if score === nothing
return nothing
else
return score + DynamicPPL.getlogjoint(trace.model.f.varinfo)
end
end

I realised the call to getlogjoint catches not only @addlogprob! but also Gibbs-conditioned variables (I think).

mhauru and others added 4 commits July 23, 2025 11:40
@mhauru
Copy link
Member Author

mhauru commented Jul 23, 2025

Most, though not quite all, of the remaining test failures are because merge and subset don't handle VariableOrderAccumulator. Do you want to merge this first and make that a separate PR?

@penelopeysm
Copy link
Member

Quite happy to!

Comment on lines 307 to +308
)
vi = DynamicPPL.setacc!!(vi, ProduceLogLikelihoodAccumulator())
Copy link
Member

@penelopeysm penelopeysm Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 337 needs the same fix as per discussion in TuringLang/DynamicPPL.jl#999.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(the fix for this is in #2629)

@penelopeysm penelopeysm mentioned this pull request Jul 26, 2025
@mhauru mhauru requested a review from penelopeysm July 28, 2025 14:48
Copy link
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thought to update the comments I left, feel free to apply those changes + merge if you're happy with them.

@penelopeysm penelopeysm merged commit c062867 into mhauru/dppl-0.37 Jul 31, 2025
7 of 30 checks passed
@penelopeysm penelopeysm deleted the mhauru/dppl-0.37-pmcmc branch July 31, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants