-
Notifications
You must be signed in to change notification settings - Fork 218
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
prob and logprob macros for simple model queries #997
Conversation
e8ec485
to
1da9b9b
Compare
Just some dispatch bugs left in Inference, then I will add tests. |
Here are the implemented and remaining features using the above model as an example:
In a future PR, I will implement:
|
Because this PR is already quite large, I will leave the syntax for creating a distribution from the model for another PR. For now, I will just finish the joint probability querying and update the docs, then this PR will be ready for review. |
Sounds good. |
63f03d2
to
2184e9d
Compare
Just docs left and maybe a couple more tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look really cool. I left a few minor remarks.
Thanks @trappmartin for your review. I will add the missing docstrings and comments while updating the docs, shortly. |
f72e93a
to
725fa6f
Compare
DistributionsAD-Zygote bug |
Docs, docs and more docs now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments for tests.
This PR feels like a Christmas present. 🎁 |
I’m happy to approve once we have the issues which discuss the special cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got no issues -- looks like @trappmartin covered pretty much everything. This is a spectacular PR -- I'm very pleased with the stuff we'll be able to do.
Some spacings are off, and the docs need an update. Then a rebase and we should be ready for a merge. Should be hopefully done by tomorrow. Would be cool to merge this on Christmas day! |
1feaf0f
to
0ed5b89
Compare
If tests pass and there are no further comments, this is ready for merge. |
The error is due to a Pkg bug. The only workarounds I know are to either limit our MCMCChains compat to pre-1.0 or to get a new release for CmdStan that supports MCMCChains 1.0. |
We can also just drop CmdStan tests for now, or merge anyways and hope for a new CmdStan release soon. |
We could also drop the MCMCChains compat to 0.4, not much changed between 0.4 and 1.0. Then we can keep the CmdStan tests in. |
We can do that but that won't fix the issue. There is no version of CmdStan that supports MCMCChains 1.0 yet. My understanding is that Pkg is struggling to downgrade MCMCChains from 1.0 to the perfectly compatible 0.4 because of a bug. So we have to either stop Pkg from thinking MCMCChains 1.0 is valid to begin with (by limiting our MCMCChains compat in Turing to pre-1.0) or provide a version of CmdStan that supports MCMCChains 1.0 and hence Pkg doesn't need to downgrade anything. This is my best understanding of the dependency error we have. |
Oh I think I misread your comment. If you mean to only support MCMCChains 0.4 then yes this should fix the error IIUC. |
Pkg error resolved, tests are running. |
Looks good so far |
027b914
to
708e358
Compare
As always @goedman comes and saves the day, thanks Rob! |
Looks like this can be merged. |
Thanks, @mohamed82008! |
This PR is based on #965. In this PR, I implemented a similar syntax to the one proposed in #989. Here is an example:
The posterior and joint probabilities are next.
Note the use of a string macro which is handled just like a normal macro except there is no syntax highlighting. The main difference under the hood is that this one doesn't go through the Julia parser to parse the input expression. The precedence of
|
in the Julia parser is quite high compared to how we want it here, so I chose not to go against the flow and just used a string macro.