-
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
Conversation
Benchmark Report for Commit 35d0be2Computer InformationBenchmark Results |
0b87d0d to
992569f
Compare
35d0be2 to
e5e98e1
Compare
|
DynamicPPL.jl documentation for PR #1037 is available at: |
Sampler and initialstepSampler and its interface
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## breaking #1037 +/- ##
============================================
+ Coverage 80.98% 81.13% +0.15%
============================================
Files 42 40 -2
Lines 3765 3722 -43
============================================
- Hits 3049 3020 -29
+ Misses 716 702 -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. |
* Bump minor version * bump benchmarks compat * add a skeletal changelog * `InitContext`, part 3 - Introduce `InitContext` (#981) * Implement InitContext * Fix loading order of modules; move `prefix(::Model)` to model.jl * Add tests for InitContext behaviour * inline `rand(::Distributions.Uniform)` Note that, apart from being simpler code, Distributions.Uniform also doesn't allow the lower and upper bounds to be exactly equal (but we might like to keep that option open in DynamicPPL, e.g. if the user wants to initialise all values to the same value in linked space). * Document * Add a test to check that `init!!` doesn't change linking * Fix `push!` for VarNamedVector This should have been changed in #940, but slipped through as the file wasn't listed as one of the changed files. * Add some line breaks Co-authored-by: Markus Hauru <markus@mhauru.org> * Add the option of no fallback for ParamsInit * Improve docstrings * typo * `p.default` -> `p.fallback` * Rename `{Prior,Uniform,Params}Init` -> `InitFrom{Prior,Uniform,Params}` --------- Co-authored-by: Markus Hauru <markus@mhauru.org> * use `varname_leaves` from AbstractPPL instead (#1030) * use `varname_leaves` from AbstractPPL instead * add changelog entry * fix import * tidy occurrences of varname_leaves as well (#1031) * `InitContext`, part 4 - Use `init!!` to replace `evaluate_and_sample!!`, `predict`, `returned`, and `initialize_values` (#984) * Replace `evaluate_and_sample!!` -> `init!!` * Use `ParamsInit` for `predict`; remove `setval_and_resample!` and friends * Use `init!!` for initialisation * Paper over the `Sampling->Init` context stack (pending removal of SamplingContext) * Remove SamplingContext from JETExt to avoid triggering `Sampling->Init` pathway * Remove `predict` on vector of VarInfo * Fix some tests * Remove duplicated test * Simplify context testing * Rename FooInit -> InitFromFoo * Fix JETExt * Fix JETExt properly * Fix tests * Improve comments * Remove duplicated tests * Docstring improvements Co-authored-by: Markus Hauru <markus@mhauru.org> * Concretise `chain_sample_to_varname_dict` using chain value type * Clarify testset name * Re-add comment that shouldn't have vanished * Fix stale Requires dep * Fix default_varinfo/initialisation for odd models * Add comment to src/sampler.jl Co-authored-by: Markus Hauru <markus@mhauru.org> --------- Co-authored-by: Markus Hauru <markus@mhauru.org> * `InitContext`, part 5 - Remove `SamplingContext`, `SampleFrom{Prior,Uniform}`, `{tilde_,}assume` (#985) * Remove `SamplingContext` for good * Remove `tilde_assume` as well * Split up tilde_observe!! for Distribution / Submodel * Tidy up tilde-pipeline methods and docstrings * Fix tests * fix ambiguity * Add changelog * Update HISTORY.md Co-authored-by: Markus Hauru <markus@mhauru.org> --------- Co-authored-by: Markus Hauru <markus@mhauru.org> * fix missing import * Shuffle context code around and remove dead code (#1050) * Delete the `"del"` flag (#1058) * Delete del * Fix a typo * Add HISTORY entry about del * Fixes for Turing 0.41 (#1057) * setleafcontext(model, ctx) and various other fixes * fix a bug * Add warning for `initial_parameters=...` * Remove `resume_from` and `default_chain_type` (#1061) * Remove resume_from * Format * Fix test * remove initial_params warning * Allow more flexible `initial_params` (#1064) * Enable NamedTuple/Dict initialisation * Add more tests * fix include_all kwarg for predict, improve perf (#1068) * Fix `include_all` for predict * Fix include_all for predict, some perf improvements * Replace `Metadata.flags` with `Metadata.trans` (#1060) * Replace Medata.flags with Metadata.trans * Fix a bug * Fix a typo * Fix two bugs * Rename trans to is_transformed * Rename islinked to is_transformed, remove duplication * Change pointwise_logdensities default key type to VarName (#1071) * Change pointwise_logdensities default key type to VarName * Fix a doctest * Fix DynamicPPL / MCMCChains methods (#1076) * Reimplement pointwise_logdensities (almost completely) * Move logjoint, logprior, ... as well * Fix imports, etc * Remove tests that are failing (yes I learnt this from Claude) * Changelog * logpdf * fix docstrings * allow dict output * changelog * fix some comments * fix tests * Fix more imports * Remove stray n Co-authored-by: Markus Hauru <markus@mhauru.org> * Expand `logprior`, `loglikelihood`, and `logjoint` docstrings --------- Co-authored-by: Markus Hauru <markus@mhauru.org> * Remove `Sampler` and its interface (#1037) * Remove `Sampler` and `initialstep` * Actually just remove the entire file * forgot one function * Move sampling test utils to Turing as well * Update changelog to correctly reflect changes * [skip ci] Make changelog headings more consistent --------- Co-authored-by: Markus Hauru <markus@mhauru.org>
what
As title. Essentially, this deletes the entirety of
src/sampler.jl.why
Because the type
Sampleris 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.samplein 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
samplewas actually too restrictive in that it doesn't allow forAbstractMCMC.step_warmupto callDynamicPPL.initialstep. This means that you can't have a customstep_warmupfor 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.