Interface Changeover #793
Merged
Interface Changeover #793
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpfiffer
commented
May 29, 2019
|
Hey @cpfiffer, the following issues have been raised for a while. It seems they are mostly interface related features. Can you take a look at them in this PR if possible?
|
|
|
xukai92
approved these changes
Sep 8, 2019
|
Seems that everything is done. Great PR! I will merge it in 24 hours if no rejection. |
willtebbutt
reviewed
Sep 9, 2019
There was a problem hiding this comment.
Really great work @cpfiffer - this has clearly been a lot of work. I've left a number of nit-picky style related comments, and also some requests for additional inline documentation.
One thing in particular: the docstrings for sample_init!, step!, and sample_end! could probably do with being improved a bit. sample_init! and sample_end! are a little bit terse, and step! produces three separate docstrings, where one would suffice. Would also be helpful to document whether or not it's expected that the user will return the modified sampler or not.
src/core/RandomVariables.jl
Outdated
| """ | ||
| islinked(vi::VT, spl::Sampler) where VT<:VarInfo | ||
|
|
||
| Returns `true` if a `VarInfo` is linked for a particular sampler `spl`. |
There was a problem hiding this comment.
A slightly more informative docstring would be appreciated. What does linked mean in this context?
There was a problem hiding this comment.
Updated to say "in the transformed space" rather than "linked", is that helpful?
There was a problem hiding this comment.
It's helpful, but maybe add a reference to some other documentation for Bijectors or something? I mean, it's clear what's happening here if you know anything about the relationship between Turing and Bijectors etc, but for a newbie it might still be a bit unclear?
There was a problem hiding this comment.
Updated to read:
islinked(vi::VT, spl::Sampler) where VT<:VarInfo
Returns true if a VarInfo is in the transformed space for a particular sampler spl.
Turing's Hamiltonian samplers use the link and invlink functions from
Bijectors.jl to map a constrained variable
(for example, one bounded to the space [0, 1]) from its constrained space to the set of
real numbers. islinked checks if the number is in the constrained space or the real space.
|
I meet a bug in the following example with thie branch using Turing
@model testmissing() = begin
n ~ Categorical(10)
l = tzeros(Int, n)
for i in 1:n
l[i] ~ Categorical(2)
end
3 ~ Normal(sum(l), 1)
end
chain = sample(testmissing(), PG(10), 1_000)gives This example works in |
Closed
|
That's a very strange bug. Seems to only happen sometimes. I'm investigating. |
xukai92
reviewed
Sep 10, 2019
|
Another 24-hour merge notifcation. |
Closed
This was referenced Sep 17, 2019
Closed
This was referenced Oct 3, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To do
Supported samplers (
src/inference/)Contributed samplers (
src/contrib/inference)[ ] SGLD[ ] SGHMC[ ] PIMH[ ] PMMH[ ] IPMCMCSummary
This will be the comprehensive PR containing all the code to change us over to using the new interface. Since the changes must be done for all the samplers pretty much all at once (we don't want half to use the new interface and the other half to use the old version), this will probably be a large PR. There is an
hmc-interface.jlfile which is not really ready for consumption yet, so you can ignore it.Currently, I have a (very) rough proof-of-concept for SMC to get myself on stable ground for the rest of the samplers. The following interface is supported:
I've introduced a couple of new things since the discussion in #746.
First, I'm attempting to unify all the samplers under the existing
Samplertype we have. Theinfofield is not going to be removed in this PR because of all theVarInfostuff -- that will be a separate undertaking. TheSamplertype now has an additional field calledstate, which contains a mutable struct that replaces the sampler-specific information likelogevidenceoreval_num. The ultimate goal is to have this go away at some point and move to a more general sampler state, but changing over to the front-end interface and changing the entire backend would be too much for one PR.Second, this isn't yet benchmarked for performance. One issue I have is how to package up the
TransitionTypes into something that has name & value information together, without too much overhead. At the moment, each step bundles up all the names fromviand all thevi.vals, but I think there's probably a better design.More to come on this.