Skip to content
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

Also pass in context as an argument to acclogp!! #563

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

torfjelde
Copy link
Member

Some samplers have very specific ways of accumulating log-probabilities which they do through overloading of the tilde-pipeline, e.g. PG in Turing overloads both assume and observe because it needs to accumulate the log-probabilities in the task local varinfo rather than the "global" varinfo.

But this means that, in certain scenarios, e.g. PG, usage of @acclogprob!! in a model will (silently) result in target the incorrect model!

One way we can fix this is to also pass in the context to @acclogprob!! (and acclogp!!), which can then alter how the log-probs are accumualted.

A few examples where this is useful:

  • Particle samplers from AdvancedPS.jl, e.g. PG in Turing.jl, can then correctly handle @addlogprob!! by accumulating the log-probabilities in the task-local varinfo rather than the "global" varinfo.
  • We can remove some hacks used in particle samplers that break composability with other contexts; in particular, instead of accumulating log-probs in the assume and returning 0 instead of the actual logp value (as is done here: https://github.com/TuringLang/Turing.jl/blob/6649f10c48917a27531214f02777408d2ab82928/src/mcmc/particle_mcmc.jl#L376), we can simply return the logp value as we do in all other implementations of assume and let the acclogp!! call in tilde_assume handle the accumulation to the task-local varinfo. This would then make particle samplers compatible with stuff like re-weighting of log-probabilities, e.g. using MiniBatchContext or the GibbsContext in New Gibbs sampler using condition Turing.jl#2099 which overrides the assume statements to instead return a conditioned value + its logprob, while avoiding hitting the observe.

src/utils.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

Pull Request Test Coverage Report for Build 6923549774

  • 11 of 11 (100.0%) changed or added relevant lines in 3 files are covered.
  • 33 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 80.284%

Files with Coverage Reduction New Missed Lines %
src/utils.jl 33 82.28%
Totals Coverage Status
Change from base Build 6906848787: 0.04%
Covered Lines: 2541
Relevant Lines: 3165

💛 - Coveralls

Copy link

codecov bot commented Nov 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c9489aa) 80.24% compared to head (1cfe231) 80.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
+ Coverage   80.24%   80.28%   +0.03%     
==========================================
  Files          25       25              
  Lines        3159     3165       +6     
==========================================
+ Hits         2535     2541       +6     
  Misses        624      624              

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

torfjelde added a commit to TuringLang/Turing.jl that referenced this pull request Nov 20, 2023
Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

Thanks @torfjelde -- this looks good. Shall we make this release breaking?

@torfjelde
Copy link
Member Author

torfjelde commented Nov 20, 2023

AFAIK it's not breaking; we're still keeping the old acclogprob!! definition too.

EDIT: bumped patch version

@yebai yebai enabled auto-merge November 20, 2023 22:35
@yebai yebai added this pull request to the merge queue Nov 20, 2023
Merged via the queue into master with commit 34be85c Nov 21, 2023
13 of 14 checks passed
@yebai yebai deleted the torfjelde/acclogp-with-context branch November 21, 2023 01:21
torfjelde added a commit to TuringLang/Turing.jl that referenced this pull request Apr 21, 2024
* initial work on the new gibbs sampler

* added tests for the new Gibbs sampler

* added tests for new Gibbs

* new Gibbs is now sampling (correctly) sequentially

* let's not overload merge just yet

* export GibbsV2 + added more samplers to the tests

* added TODO comment

* removed lots of varinfo related merging functionality that is now
available in DynamicPPL

* shifting some code around

* removed redundant constructor for GibbsV2

* added GibbsContext which is similar to FixContext but also computes
the log-prob of the fixed variables

* adopted the rerun mechanism in Gibbs for GibbsV2, thus fixing the
issues with some of the tests for GibbsV2

* broken tests are no longer broken

* fix issues with dot_tilde_* impls for GibbsContext

* fix for dot_tilde_assume when using GibbsContext

* fixed re-running of models for Gibbs sampling properly this time

* added new gibbs to tests

* added some further comments on why we need `GibbsContext`

* went back to using `DynamicPPL.condition` rather than using custom
`GibbsContext` while we wait for
TuringLang/DynamicPPL.jl#563 to be merged

* add concrete comment about reverting changes for `gibbs_condition`

* Update test/mcmc/gibbs_new.jl

Co-authored-by: Hong Ge <[email protected]>

* fixed recursive definition of `condition` varinfos

* use `fix` instead of `condition`

* Revert "use `fix` instead of `condition`"

This reverts commit f87e2d1.

* rmeoved unnused symbol

* Revert "went back to using `DynamicPPL.condition` rather than using custom"

This reverts commit 53bd707.

* bump compat entry of DynamicPPL so we can overload acclogp!

* update assume for SMC samplers to make use of new `acclogp!`

* added proper impl of acclogp!! for SMC samplers + made accessing
task local varinfo and rng a bit nicer

* added experimental module and moved gibbs to it

* fixed now-inccorect references in new gibbs file

* updated gibbs tests

* moved experimental gibbs tests

* updated tests to include experiemntal tests

* removed refrences to previews tests of experimental Gibbs sampler

* removed solved TODO

* added a comment on `reconstruct_getvalue` usage

* bump patch version

* added comments on future work

* Update test/experimental/gibbs.jl

* fixed bug where particle samplers didn't properly account for
weightings of logpdf, etc.

* relax atol for a numerical test with Gibbs a bit

* fixed bug with `AbstractDict` constructor for experimental `Gibbs`

* aaaalways link the varinfo in the new Gibbs sampler, just to be sure

* add test to cover recent improvement to `DynamicPPL.subset`

ref: TuringLang/DynamicPPL.jl#587

* bump compat entry for DynamicPPL

* added some docstrings

* fixed test

* fixed import

* another attempt at fixing tests

* another attempt at fixing tests

* attempt at fix tests

* forgot something in previos commit

* cleaned up the experimental Gibbs sampler a bit

* removed accidentaly psuedocode inclusion

* Apply suggestions from code review

* relaxed olerance in one MH test a bit

* bump patch version

---------

Co-authored-by: Hong Ge <[email protected]>
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