-
Notifications
You must be signed in to change notification settings - Fork 230
Move external sampler interface to AbstractMCMC #2704
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
base: breaking
Are you sure you want to change the base?
Conversation
|
Turing.jl documentation for PR #2704 is available at: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #2704 +/- ##
============================================
- Coverage 86.45% 85.52% -0.94%
============================================
Files 21 21
Lines 1418 1409 -9
============================================
- Hits 1226 1205 -21
- Misses 192 204 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -1,5 +1,5 @@ | |||
| """ | |||
| ExternalSampler{S<:AbstractSampler,AD<:ADTypes.AbstractADType,Unconstrained} | |||
| ExternalSampler{Unconstrained,S<:AbstractSampler,AD<:ADTypes.AbstractADType} | |||
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.
Moved this type parameter earlier so that we can dispatch on it more easily.
Note
This PR is pretty much ready to go, but requires TuringLang/AbstractMCMC.jl#182 before it can be properly merged, hence marking as draft.
This PR moves external sampler interface functions like
getparamsandgetstatsout of Turing. It's really a proper continuation of #2616, which I messed up quite badly by merging into a patch release and had to revert. This PR therefore targets breaking.The twin PR above adds them to AbstractMCMC. Once this is done, anyone who defines an external sampler will only need to depend on AbstractMCMC (which they presumably already do, because it is a sampler) and LogDensityProblems (which is already a dep of AbstractMCMC). No need for a Turing extension.
For an example implementation, see the test mock in this PR (note no Turing dependencies required):
Turing.jl/test/mcmc/external_sampler.jl
Lines 20 to 74 in 06752c4