-
Notifications
You must be signed in to change notification settings - Fork 32
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
Standardising APIs between compiler and inference methods #16
Comments
@willtebbutt @xukai92 @model demo(y) = begin
5 ~ Normal(0,1) # rv1
y ~ Normal(0,1) . # rv2
end
gdemo()() # sample both `rv1` and `rv2` from the prior, ingoring `rv1=5`
gdemo(2)() # sample from the posterior This leads to consistent behaviour and should be intuitive too. |
I see. But what I mean by this issue is to actually have some concrete functions generated by evaluating Currently behaviour: @model demo(y) = begin
5 ~ Normal(0,1) # rv1
y ~ Normal(0,1) . # rv2
end
mf = gdemo()
mf() # => sampling from prior But instead we could have @model demo(y) = begin
5 ~ Normal(0,1) # rv1
y ~ Normal(0,1) . # rv2
end
mf = gdemo(???) # this returns mf::UniqueRuntimeModelType
# and create some function definitions:
logpdf(::UniqueRuntimeModelType, vi) = ... # (1) return logpdf by evaluating using vi
rand(::UniqueRuntimeModelType) = ... # (2) gdemo()() in your example
sample(::UniqueRuntimeModelType) = ... # (3) gdemo(2)() in your example I guess it could also potentially give more space for performance because during sample we are explicitly using the specific version of |
@yebai your proposal has the issue that the behaviour of the following programme becomes ambiguous: @model demo() = begin
5 ~ Normal(0, 1)
end
demo() # what does this do? This makes me nervous. On the one hand, this programme is weird, on the other it's a valid Turing programme, so we should ensure that the behaviour is well-defined. My proposal below will mostly resolve this. @xukai92 I completely agree regarding the format of
|
We can give an error if the model has no variables.
This can be just Btw, after the new refactor, if you call |
Agreed, but this programme definitely has a random variable. It's anonymous, but it's definitely there, and if we define sample-from-prior functionality, then presumably something should happen.
This is probably fine for now, but I'm not sure it's appropriate long-term for the same reasons as above.
I like this. We would have to think carefully about how the |
Is there a practical application of sampling this anonymous random variable, or is it just for theoretical appeal? Note that implementation-wise, there is nothing random about a model with no explicit random variables; every time you run the model, you will get exactly the same thing, hence my difficulty digesting this anonymous random variable concept. I can see how
Sounds good. |
Note that currently, we just ignore any numbers or arrays on the LHS of a Edit: by ignore I mean that we don't treat it as an explicit variable, but it still affects the |
I'm just looking for consistency. I don't have a particular application in mind :)
Not if you run the model in sample-from-prior mode.
This general kind of thing would be a good change. If the model is constructed in the regular way, it should be a |
@model demo() = begin
5 ~ Normal(0,1)
end
model = demo()
logp = model(Turing.VarInfo(), Turing.SampleFromPrior())
all(isequal(logp), (model(Turing.VarInfo(), Turing.SampleFromPrior()) for i in 1:100))
|
This should be totally possible when constructing the |
Sorry, I should have been clearer. I was referring to what I think should be going on rather than what actually happens at the minute. You're totally correct regarding the current behaviour. It's my feeling that if we implement a sampler-from-prior mode, then we should take these anonymous random variables more seriously and treat them more like named random-variables. |
I don't foresee a problem handling these. I can try a proof-of-concept after the |
Thanks for cc‘ing me. I’m pretty much in favour of the However, I’m not sure how inference with VI would fit into those syntaxes to stay consistent without having too many entry points for the user. |
My thinking here is that VI works fine with the
Again, my feeling here is that we wouldn't need the
As am I :) A minor stylistic thing, @yebai what would your thoughts be on making the "correct" thing to do be |
Reading through your explanation of |
Here is a summary of some proposals in this issue (biased towards personal aesthetic)
This would provide a relatively well-defined interface between the compiler and inference methods (e.g. variational methods and sampling methods). For variational methods, the most important API is the
A long term goal is to build up a clear separation between the compiler and inference methods (see TuringLang/Turing.jl#456). Such that inference methods can be implemented in separate packages with a dependency on Turing. Anything missing? |
How about enabling this alias with support to sampling algorithm as well.
|
mf()
and implement explicit rand
and logpdf
functions for mf
How about |
I don't know that we actually need a supertype here. I mean, is there going to be a fallback that can be applied to |
Let's stick with |
I suggest during the model compilation we generate 2 functions inside
|
@mohamed82008 I think this is a good idea. Thanks! I'll take a closer look into this asap. |
@yebai I had some time to read through your proposal. It sound quite reasonable to me. However, I have a few questions / remarks.
In favour of what Mohamed suggested and the latest discussions on model criticism, it might be good to have:
As proposed by Mohamed we could do this by generating respective functions in the model macro. |
Could someone please link the relevant discussions here? @trappmartin could you please explain the above proposal in a bit more depth? What are the differences between each of the functions? What are @mohamed82008 could you please expand on your proposal? It's not obvious to me why we need a separate model construction to perform maximum likelihood / when we would actually be interested in performing maximum likelihood in our context: if the user has gone to the trouble to specify priors over parameters, why would they wish to ignore them? |
|
I don't think we want maximum likelihood. But I agree it would be good to provide an interface to compute the log likelihood. |
Sorry, I'm not familiar with the
If we're going to rename |
Uh, sorry that was a typo. I meant |
haha fair enough. Glad we're on the same page with this.
Is there a good reference for this, or is this something that should be obvious? |
See the paper by A. Gelman, J. Hwang, and A. Vehtari on Understanding predictive information criteria for Bayesian models for example. |
I just thought it would be cool to compare ML, MAP, VI and MCMC without changing the model definition! But @trappmartin has a lot more interesting use cases :) |
you're right - # s::Sampler ==> s::Selector
logp = model(Turing.VarInfo(), s::Sampler) Since
@mohamed82008 I'm not sure I understand the point here. For MAP and VI based learning, all we need is
It seems I agree it's helpful to have some support for model evaluation and criticism, but I prefer to keep the standard API between compiler and inference minimal. These additional features can be supported in a separate module (e.g. |
Yes, seems more telling to me than
Good point. I'll try to do so as it seems relevant for several people. |
Is there anything here that has not already implemented in the meantime, or couldn't be moved to the discussions at AbstractPPL? |
Nothing that I know of. Please feel free to close, and/or move some useful bits to |
See https://github.com/TuringLang/Turing.jl/issues/634#issuecomment-471339521 for a summary.
The text was updated successfully, but these errors were encountered: