Skip to content
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

change defaults: @addlogprob! not included in PriorContext() #580

Open
thorek1 opened this issue Feb 21, 2024 · 5 comments
Open

change defaults: @addlogprob! not included in PriorContext() #580

thorek1 opened this issue Feb 21, 2024 · 5 comments

Comments

@thorek1
Copy link

thorek1 commented Feb 21, 2024

Would it be possible to change the default behavior for @addlogprob! and not have it included in the PriorContext()

I understand the current solution is to do this:

if DynamicPPL.leafcontext(__context__) !== Turing.PriorContext()
    Turing.@addlogprob! myloglikelihood(x, μ)
end

as in the default is that it is included.

In my use case (see this issue) it would make the syntax much more user-friendly for end-users

@devmotion
Copy link
Member

IMO (at first glance at least) the current default is reasonable since @addlogprob! just indicates that something is added to the joint log density that is computed by default.

That being said I think the syntax could be improved: #390

@thorek1
Copy link
Author

thorek1 commented Feb 21, 2024

what about setting up another macro (e.g. @addlogprob_noprior) that does this?

the request is triggered by the fact that some backends care about the context. I agree, from a user perspective that this should not be relevant. hence, my desire to have a simple one-line solution

@devmotion
Copy link
Member

I think #390 would be cleaner than a separate macro - in Turing, every tilde statement is either an assumption ("prior") or an observation ("likelihood"), so stating explicitly to which category the custom log density calculation belongs would rule out any confusion (and could ideally be treated correctly then also when someone implements eg a custom prior context, which @addlogprob_noprior etc. would not know about).

@torfjelde
Copy link
Member

I'm very happy with adopting the syntax in #390 tbh. Shouldn't be too difficult to implement either?

@thorek1
Copy link
Author

thorek1 commented Feb 21, 2024

#390 is suggesting to have an option after the macro call, right? if so, sounds good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants