-
Notifications
You must be signed in to change notification settings - Fork 36
Remove Sampler
and its interface
#1037
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
Benchmark Report for Commit 35d0be2Computer Information
Benchmark Results
|
0b87d0d
to
992569f
Compare
35d0be2
to
e5e98e1
Compare
DynamicPPL.jl documentation for PR #1037 is available at: |
Sampler
and initialstep
Sampler
and its interface
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## breaking #1037 +/- ##
============================================
+ Coverage 82.40% 82.57% +0.17%
============================================
Files 42 40 -2
Lines 3791 3748 -43
============================================
- Hits 3124 3095 -29
+ Misses 667 653 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM
Thanks @sunxd3! Will hold off on merging until we're happy with TuringLang/Turing.jl#2676 so as to not pull the rug from under it... |
Absolutely, I was going to mention this, but thought you had better judgement on the ordering of things. So no problem at all. |
what
As title. Essentially, this deletes the entirety of
src/sampler.jl
.why
Because the type
Sampler
is gone, and also because DynamicPPL has no more actual samplers (in the past there wereSampleFromPrior()
andSampleFromUniform()
that were actually samplers) that means we don't need to declare generic methods forAbstractMCMC.sample
in DynamicPPL. That also means that the supporting interface used to define e.g. keyword argument defaults can also go.Of course, it doesn't go completely: it just gets shifted upwards to Turing. In my opinion that makes a ton more sense anyway because that's where the concrete implementations of
sample()
are written. The current status is that these functions are defined in DynamicPPL but no methods are defined anywhere in DPPL, only in Turing. So may as well move the actual function there.On top of this, the default implementation for
sample
was actually too restrictive in that it doesn't allow forAbstractMCMC.step_warmup
to callDynamicPPL.initialstep
. This means that you can't have a customstep_warmup
for Turing samplers.where now
My plan for which function goes where is outlined in the changelog:
DynamicPPL.jl/HISTORY.md
Lines 45 to 55 in be0d2ad
For the upcoming release of Turing v0.41 I will just do a straight copy paste of the code here, instead of doing any larger refactoring. But I would like this PR to be in DPPL 0.38 so that we don't have to cut a new minor version just to delete this.