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

Support for computing log density of distributions #297

Merged
merged 5 commits into from
Jun 26, 2023
Merged

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented Jun 14, 2023

Merge after #296 as it contains those commits (see work plan in mrc-4182).

See mrc-4324...mrc-4326 for diff of just these changes

This PR computes the log density for some distributions (not many, and not exhaustive unlike #296 as this needs to mirror dust for now and there are a couple of interface issues to nail down later). It is intended to be enough to implement some proof-of-concept models more than anything actually very useful.

Note that there's a difference in how we hit this logic vs the expectations - with the expectations we analyse an expression and then every time we hit a r* function (e.g., rpois) we replace the expression with a new one but most expressions pass through unchanged and we only alter things if they match our table of rules. Here, all compare expressions have the same type of call (compare(d) ~ poisson(lambda) etc) which means that we don't do any analysis of the expressions we just start substituting directly.

Note that this also exposes a bit of a pain that we write

x <- rpois(lambda)
compare(d) ~ poisson(x)

which uses entirely different forms for the two calls. See mrc-4182 for more on that

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2ac75e4) 100.00% compared to head (942e2dd) 100.00%.

❗ Current head 942e2dd differs from pull request most recent head 08963fa. Consider uploading reports for the commit 08963fa to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #297   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           48        47    -1     
  Lines         5517      5521    +4     
=========================================
+ Hits          5517      5521    +4     
Impacted Files Coverage Δ
R/differentiate-support.R 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@richfitz richfitz marked this pull request as ready for review June 15, 2023 20:45
Copy link
Contributor

@r-ash r-ash left a comment

Choose a reason for hiding this comment

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

Looked through this with Wes, Emma and Mantra.

We think the code looks good! But we're a little confused about when we would want to use the deterministic version. Couldn't find any docs in dust about this either but maybe I am not looking in the right place

Copy link
Contributor

@weshinsley weshinsley left a comment

Choose a reason for hiding this comment

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

OK - looks good and I think I understand what this adds - check version before merging

@richfitz
Copy link
Member Author

We think the code looks good! But we're a little confused about when we would want to use the deterministic version. Couldn't find any docs in dust about this either but maybe I am not looking in the right place

I think this comment really goes with #296 ?

The answer in any case is that this has proven a useful approximation as it runs way faster (but captures less of the dynamics). It's not yet clear exactly how the two relate to each other, and how you should move from one to another - some proper work needs doing on this at some point.

The only docs are two lines here. I probably need to work with the science side to write up some good motivating examples, and that might be easier once they finish with a deterministic-vs-stochastic paper they're working on.

richfitz and others added 5 commits June 26, 2023 14:57
Co-authored-by: M-Kusumgar <98405247+M-Kusumgar@users.noreply.github.com>
@r-ash
Copy link
Contributor

r-ash commented Jun 26, 2023

We think the code looks good! But we're a little confused about when we would want to use the deterministic version. Couldn't find any docs in dust about this either but maybe I am not looking in the right place

I think this comment really goes with #296 ?

Agh yes it does! Thanks

@richfitz richfitz merged commit 4f9f69e into master Jun 26, 2023
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.

4 participants