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

Add Capability to Marginalize Likelihood #539

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

pbauman
Copy link
Member

@pbauman pbauman commented Feb 2, 2017

This follows up on #528 to allow the marginalization of a "marginal parameter space". That is, if instead of your model looking like f(m), it looks like f(m,q) and you want to marginalize over q, you'll now be able to do that.

The code is written, but it will be a couple of more days before I can have time to write tests. That said, i wanted to open this up now to get feedback on the interface and on the clarity of the documentation (since the integration over the marginal space can be done in a couple of ways). As such, I'm labeling as do not merge until I push some tests (and squash in any fixes for bugs identified by the testing).

@dmcdougall
Copy link
Member

That was fast.

@codecov-io
Copy link

codecov-io commented Feb 2, 2017

Codecov Report

Merging #539 into dev will increase coverage by 0.01%.

@@            Coverage Diff            @@
##             dev     #539      +/-   ##
=========================================
+ Coverage     56%   56.02%   +0.01%     
=========================================
  Files        186      186              
  Lines      18035    18050      +15     
=========================================
+ Hits       10101    10112      +11     
- Misses      7934     7938       +4
Impacted Files Coverage Δ
...c/stats/src/GaussianLikelihoodDiagonalCovariance.C 92.85% <100%> (-0.9%)
...elihoodBlockDiagonalCovarianceRandomCoefficients.C 96.66% <100%> (-0.21%)
...ts/src/GaussianLikelihoodBlockDiagonalCovariance.C 88.88% <100%> (-0.77%)
src/stats/src/GaussianLikelihoodFullCovariance.C 93.33% <100%> (-0.79%)
src/stats/src/GaussianLikelihoodScalarCovariance.C 100% <100%> (ø)
...aussianLikelihoodFullCovarianceRandomCoefficient.C 94.73% <100%> (-0.51%)
src/stats/src/LikelihoodBase.C 78.37% <85.18%> (+18.37%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 199283f...e339a52. Read the comment docs.

@pbauman
Copy link
Member Author

pbauman commented Feb 2, 2017

:) That code was written together with #528 and #532. Instead of just letting it sit until I write the tests, I figured I could start getting feedback on the API and documentation now.

We want to factor out the model evaluation from the actual computation
of the log-likelihood for forthcoming additions of "marginalized
parameters". So, we add lnValue() to LikelihoodBase, where it
calls evaluateModel() first, then we pass the model evaluation
to lnLikelihood(). Now, subclasses implement lnLikelihood with
the modelOutput as an input parameter to the function. We don't
pass modelOutput as const so that it can be overwritten to avoid
copying the modelOutput, preserving the behavior that was there
before. I'm not sure I like this behavior, but it's also a protected
function that's working a vector created internal to the class
hierarchy.
This is laying the groundwork for having the likelihood do
marginalization for the user. This add the neccessary data and
constructor to LikelihoodBase. In particular, we'll need the joint
pdf of the marginal parameter space and the user-constructed
integration rule for integrating over that space.
The user will need to override this method for doing marginalization
in the likelihood. In particular, this method will also pass the
current value of the marginal parameters in addition to the inference
parameters. This will be the user function that gets called if
a marginal parameter space and integration rule have been provided.
If we detect the user has supplied a marinal parameter pdf, then
we instead will use the user-supplied integration rule to integrate
over the parameter space, calling the user-overrideen evaluateModel()
method that accepts marginal parameters as well as inference parameters.
@pbauman
Copy link
Member Author

pbauman commented Feb 9, 2017

OK, I have two tests now. This is ready as far as I'm concerned. Feedback welcome @dmcdougall, in particular is the documentation clear? I'm a little worried the marg_pdf_is_weight_func in the new constructor might be confusing.

I should note that I intend to open an issue after this is merged because one thing I'd like to do is generalize this to parallelize the quadrature loop. But that's a longer discussion. As is, this is just evaluating one quadrature point at a time in the marginalization step, which is fine if your model is cheap.

This is testing the case where we have a uniform RV for the marginal
parameters. The likelihood is just f(m) (data =0, effectively uniform
pdf) for ease of analytical integration for the test.
This one is now testing where we treat the marginal parameter pdf
as the weighting function in the quadrature rule. Here, we use
a zero mean/unit variance Gaussian for the 2D marginal parameter space
and then use Gauss-Hermite quadrature.
@pbauman
Copy link
Member Author

pbauman commented Feb 10, 2017

Had forgotten to implement the new constructor for the Gaussian likelihood subclasses. Pushed in a commit that does so.

@pbauman
Copy link
Member Author

pbauman commented Feb 14, 2017

Labeling do not merge again. Even though my tests are passing my application that is using this capability is not behaving as expected and I suspect one or several bugs still lingers here. Will update when its sorted, one way or another.

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

Successfully merging this pull request may close these issues.

3 participants