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

remove lp__ argument from _lpmf functions #397

Merged
merged 1 commit into from
Dec 17, 2019

Conversation

nhuurre
Copy link
Collaborator

@nhuurre nhuurre commented Dec 6, 2019

A quick fix for issue #335

See my comment on the issue for reasons why I chose this approach rather than the one suggested by the issue title.

Copyright holder: Niko Huurre

By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:

@seantalts
Copy link
Member

I read the issue but I'm not sure what the approach here is - the legal argument you laid out on the issue might suggest we no longer allow all functions containing the characters lp to access lp, right? But the code here is all in the backend - I would have expected to see some changes to the front end to print pretty error messages when the user wants to do that but they aren't allowed to anymore. Could you write a few sentences explaining what the approach is? Thanks!

Copy link
Member

@seantalts seantalts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think I get it - the front-end was already doing the right thing, only the backend had the spec wrong for functions containing lp. So the stanc3 frontend is already rejecting the models that ask for access to lp in a user-defined distribution, but the backend was following the stan2 compiler in part of its generated code and trying to pass along lp__ to all distribution functions always.

Great fix, thank you!

@seantalts
Copy link
Member

(PS I really enjoyed the name of this branch :P)

@nhuurre nhuurre deleted the issue-335-theres-no-lp-in-lpmf branch December 17, 2019 22:25
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

Successfully merging this pull request may close these issues.

2 participants