[Merged by Bors] - Extend docstring of @addlogprob!
#389
Conversation
| julia> @model function demo(x) | ||
| μ ~ Normal() | ||
| if DynamicPPL.leafcontext(__context__) !== PriorContext() |
There was a problem hiding this comment.
The leafcontext is not exported and may change. So this example might be fragile. In the future, wt would be better to avoid calling these low-level internal APIs to just get the current (primitive) context. But this should be addressed in separate PRs.
cc @rikhuijzer you might want to take a look at how context works in DynamicPPL.
There was a problem hiding this comment.
Yes, I was not super happy about bringing up leafcontext here - the purpose of the macro is to ensure that users don't have to use internal variable names and functions, so this is really not in line with our goals. Maybe we should just add a warning that it's always accumulated and one has to work with internal unstable fumctionality if that's not OK?
There was a problem hiding this comment.
I think it is okay to keep the example - it is useful for the users, but add a (big) warning that this interface is internal, and may change in the future.
A side note: we would like to improve the context design, together with the model macro implementation, in the longer term. Hopefully, these improvements will remove the requirement of using leafcontext.
There was a problem hiding this comment.
Another alternative (or addition) would be separate macro (or a switch?) if the manual log probability corresponds to an observe statement. (One could also add something for assume statements explicitly, but I guess it is less common since then one also should add a sample to the varinfo - so maybe the observe behaviour should be the default?)
There was a problem hiding this comment.
It does not feel critical to have multiple versions of @addlogprob! for now. Most people only use it inside MH or HMC samplers. But there is a need, we can add support for that later.
There was a problem hiding this comment.
I'm mainly concerned about the, in most cases, incorrect results of logprior currently - which is the main reason for this annoying leafcontext approach. I think this could be fixed by identifying it with a custom observe statement instead of an arbitrary modification of the log probability. One could eg. change the macro syntax to
@addlogprob! val [assume=false] [observe=true]Then it would still be possible to unconditionally modify the log probability but one could also limit it easily to evaluations that include the likelihood or the prior.
There was a problem hiding this comment.
But in any case this is out of scope for this PR, I think.
|
Very nice example, thanks @devmotion! |
Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com>
|
bors r+ |
|
Very nice!! 🚀 |
|
Pull request successfully merged into master. Build succeeded: |
bors
bot
changed the title
@addlogprob!@addlogprob!
No description provided.