-
Notifications
You must be signed in to change notification settings - Fork 0
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 combining models #14
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 39 40 +1
Lines 1689 1689
=========================================
Hits 1689 1689 ☔ View full report in Codecov by Sentry. |
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.
Nice one, was pretty clear, couple small suggestions!
R/combine.R
Outdated
call = call) | ||
} | ||
|
||
## Next we need to check that the model with a sample has the full set of |
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.
## Next we need to check that the model with a sample has the full set of | |
## Next we need to check that the model with a sample has the full set of parameters |
paste("Can't create a direct_sample from these models as '{name}' does", | ||
"not contain all parameters in both models"), |
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.
im not sure that the name "a" or "b" is useful to the user, i found it actively confusing in the test for this, can we do a
`+.mcstate_model` <- function(x, y,
names = c(deparse(substitute(x)),deparse(substitute(y))))
and pass this down into this function so that we can reference the variable that they tried to sum?
509e01b
to
24e47b5
Compare
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.
Looks neat - some tiny doc typos!
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.
Nice one, i like the "lhs" and "rhs", much clearer!
Co-authored-by: Wes Hinsley <[email protected]>
This PR adds support for combining models via adding their log densities. The basic idea is to support the fairly common case that we have a pair of models, one representing the (log) priors and the other representing the (log) likelihood that we want to join together to make a posterior. We can then construct these however we want and combine them with
mcstate_model_combine
(or+
which is syntactic sugar for this).The way that different model components combine is described in some detail in the docs but briefly:
Because I was finding it hard to work with, I've changed domain slightly to include parameter names.