-
Notifications
You must be signed in to change notification settings - Fork 32
Implement missing tilde_assume method #904
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
Conversation
Benchmark Report for Commit 8402289Computer Information
Benchmark Results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
==========================================
- Coverage 85.05% 85.01% -0.05%
==========================================
Files 35 35
Lines 3922 3924 +2
==========================================
Hits 3336 3336
- Misses 586 588 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 14698049682Details
💛 - Coveralls |
7ca9a20
to
8402289
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
In Turing.jl v0.37, when a Gibbs sampler is used without specifying component samplers for all variables, the variables without samplers are sampled once at the beginning and don't ever move from there. See TuringLang/Turing.jl#2536.
This patch is required to preserve that behaviour - without this patch it will throw a MethodError as it was trying to look for the newly added method. Specifically it was this line in Turing.jl that was calling the missing method https://github.com/TuringLang/Turing.jl/blob/fc32e10bc17ae3fda4d7e825b6fde45dc7bdb179/src/mcmc/gibbs.jl#L162-L168
Should hopefully be simple enough to approve -- though I'm very much not a fan of how the N optional arguments force us to define 2^N methods, and it may be worth thinking about how to deal with this in a better way long-term. See also #720.