-
Notifications
You must be signed in to change notification settings - Fork 219
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
Allow usage of AbstractSampler
#2008
Conversation
As demonstrated in #2011 it seems supporting this properly is somewhat non-trivial, and so for the moment I've decided to just introduce a simple The annoying thing (other than it being ugly) is that the user has to wrap the |
Pull Request Test Coverage Report for Build 5457548017Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #2008 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 21 22 +1
Lines 1426 1458 +32
======================================
- Misses 1426 1458 +32
☔ View full report in Codecov by Sentry. |
AbstractSampler
AbstractSampler
I believe the tests are sometimes failing simply because of numerical issues. Some of the test models, e.g. |
* no tests * more tol
struct TuringTransition{T,NT<:NamedTuple,F<:AbstractFloat} | ||
θ::T | ||
lp::F | ||
stat::NT | ||
end | ||
|
||
function TuringTransition(vi::AbstractVarInfo, t) | ||
theta = tonamedtuple(vi) | ||
lp = getlogp(vi) | ||
return TuringTransition(theta, lp, getstats(t)) | ||
end | ||
|
||
metadata(t::TuringTransition) = merge((lp = t.lp,), t.stat) | ||
DynamicPPL.getlogp(t::TuringTransition) = t.lp | ||
|
||
state_to_turing(f::DynamicPPL.LogDensityFunction, state) = TuringState(state, f) | ||
function transition_to_turing(f::DynamicPPL.LogDensityFunction, transition) | ||
θ = getparams(transition) | ||
varinfo = DynamicPPL.unflatten(f.varinfo, θ) | ||
# TODO: `deepcopy` is overkill; make more efficient. | ||
varinfo = DynamicPPL.invlink!!(deepcopy(varinfo), f.model) | ||
return TuringTransition(varinfo, transition) | ||
end | ||
|
||
# NOTE: Only thing that depends on the underlying sampler. | ||
# Something similar should be part of AbstractMCMC at some point: | ||
# https://github.com/TuringLang/AbstractMCMC.jl/pull/86 | ||
getparams(transition::AdvancedHMC.Transition) = transition.z.θ | ||
getstats(transition::AdvancedHMC.Transition) = transition.stat | ||
|
||
getparams(transition::AdvancedMH.Transition) = transition.params | ||
getstats(transition) = NamedTuple() |
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.
@torfjelde Isn't this obsolete now that #2026 was merged?
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.
I think so. @JaimeRZP, can you do a follow-up PR to unify these Transition
types?
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.
Yes it is, which I was aware of; me and Jaime wanted to wait until that had been merged before merging this PR.
I was planning to incorporate those changes into this PR before merging. We're also missing a version-bump.
I'd appreciate it if we left merging of a PR to the person who opened it, unless otherwise explicitly stated. In particular now when it's just a matter of days before I'll be back in full development capacity again. This has happened quite few times now :/
Also, it's not like this PR needs to be merged to be able to develop other functionality; it's easy enough to just depend on the branch directly.
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.
I was under the impression that @JaimeRZP needs this to be released. Sorry for the rush -- hopefully, it didn't break anything! We can always add more changes in a follow-up PR.
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.
This is a very rough idea of how we could go about supporting
AbstractMCMC.AbstractSampler
to reduce the necessary glue code.With this, the following works: