pymc-devs / pymc-experimental

https://pymc-experimental.readthedocs.io
Other
72 stars 46 forks source link

Implement Laplace (quadratic) approximation #345

Closed carsten-j closed 4 days ago

carsten-j commented 1 month ago

This is an early version of q quadratic approximation implementation that I have developed while reading Statistical Rethinking by Richard McElreath.

There is a short discussion about this in the issue and maybe @theorashid can help with feedback of this draft PR.

This work is partly based on the Python package pymc3-quap but pymc3-quap is based on PYMC3 and a lot happend bewteen version 3 and 5 of PYMC. Optimizers works better when provided with a good initial guess and hence a (optional) starting point has been added to function arguments. Please see Github for a discussion about the differences between PYMC version 3 and 5 for computing the Hessian.

carsten-j commented 1 month ago

I am looking for the best way to return not just a posterior sample distribution but also the mean vector and covariance matrix of the Gaussian distribution. Any suggestion for this. So far my only idea is to add another section to the inferenceData returned containing this information. Thoughts on this?

zaxtax commented 1 month ago

I'm not sure the InferenceData is the best place to put it. We should copy whatever we do with Variational Inference

On Sun, 2 Jun 2024, 13:05 Carsten Jørgensen, @.***> wrote:

I am looking for the best way to return not just a posterior sample distribution but also the mean vector and covariance matrix of the Gaussian distribution. Any suggestion for this. So far my only idea is to add another section to the inferenceData returned containing this information. Thoughts on this?

— Reply to this email directly, view it on GitHub https://github.com/pymc-devs/pymc-experimental/pull/345#issuecomment-2143803800, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACCUM45EIPXABROOJPVCTZFL353AVCNFSM6AAAAABIT3F2F2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTHAYDGOBQGA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

aloctavodia commented 1 month ago

Historically, Inferencedata has been focused on mcmc. But we have discussed a few times extend it to better handle other inference methods, like SMC or variational methods. It just that there has not been enough momentum to agree and implement and schema that works for those methods.

carsten-j commented 1 month ago

@zaxtax and @aloctavodia are you saying that I should not return inferencedata at all or just not return the gaussian mean and covariance in the inferencedata object? I am new to both PYMC and Bayesian statistics so I do not know the history of this package. Best, Carsten

zaxtax commented 1 month ago

Oh, it's more that we haven't decided how to handle this within the library. Don't treat this as a blocker, though we should raise it for discussion more broadly

On Sun, 2 Jun 2024, 19:04 Carsten Jørgensen, @.***> wrote:

@zaxtax https://github.com/zaxtax and @aloctavodia https://github.com/aloctavodia are you saying that I should not return inferencedata at all or just not return the gaussian mean and covariance in the inferencedata object? I am new to both PYMC and Bayesian statistics so I do not know the history of this package. Best, Carsten

— Reply to this email directly, view it on GitHub https://github.com/pymc-devs/pymc-experimental/pull/345#issuecomment-2143944495, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACCUKGG7KIW4SVLMG6ZLLZFNGB7AVCNFSM6AAAAABIT3F2F2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTHE2DINBZGU . You are receiving this because you were mentioned.Message ID: @.***>

twiecki commented 1 month ago

CC @ferrine

aloctavodia commented 1 month ago

Oh, it's more that we haven't decided how to handle this within the library. Don't treat this as a blocker, though we should raise it for discussion more broadly

exactly, just saying that if necessary InferenceData can be extended.

ricardoV94 commented 1 month ago

Suggestion, include two groups in the returned inferencedata:

  1. A fit group that includes the mean and covariance of the laplace fit, and
  2. a posterior group that includes draws from this fit, and has all the bells and whistles like dimensions, deterministics, etc... Include the default extra groups like observed and constant data. This will look just like a fit from mcmc sampling. This can be disabled by the user by setting draws = 0

We could even try different fits from distinct initialization points (optionally) and save those as distinct "chains" in the fit and corresponding posterior groups. Although usually multiple initialization are used with the goal of finding the best fit, they could still be useful to detect multi-modality / pervasiveness of local optima.

ricardoV94 commented 1 month ago

@carsten-j PR looks great! I left some comment above

carsten-j commented 4 weeks ago

Thanks you @ricardoV94 and @twiecki for the review comments. I believe that all of them expect one has been fixed. I have not figured out how to use remove_value_transforms. I tried to browse through PYMC source code but that did not really help.

ricardoV94 commented 3 weeks ago

Thanks you @ricardoV94 and @twiecki for the review comments. I believe that all of them expect one has been fixed. I have not figured out how to use remove_value_transforms. I tried to browse through PYMC source code but that did not really help.

The docs contains code example: https://www.pymc.io/projects/docs/en/stable/api/model/generated/pymc.model.transform.conditioning.remove_value_transforms.html

carsten-j commented 3 weeks ago

I should have mentioned that I did read the doc and looked at the example. But I have not been able to figure out how to apply it to my case. I will try again ...

ricardoV94 commented 3 weeks ago

To be able to use it inside the model context, it will need this change to get merged first: https://github.com/pymc-devs/pymc/pull/7352

But you should be able to already test by doing the object way with pm.fit(..., model=model) outside of the model context

carsten-j commented 3 weeks ago

@ricardoV94 I figured out how to replace the for loop with remove_value_transforms. Is the PR ready for merge or are there additional review comments?

carsten-j commented 3 weeks ago

@ricardoV94 it does work but I have updated the code according to your comment. It makes sense and I am still learning about PYMC :-)

carsten-j commented 3 weeks ago

@ricardoV94 tests are failing but I do not see any relationship between the failing tests and this new Laplace feature. It there a general problem with tests?

carsten-j commented 3 weeks ago

@ricardoV94 tests are still failing. I am not sure what to do here as I do not see why these tests should fail due to my PR. Please advice.

ricardoV94 commented 3 weeks ago

@carsten-j I can confirm those failures are unrelated to your changes, and related to changes in PyMC or other dependencies. We'll have to fix them in a separate PR

carsten-j commented 3 weeks ago

Thanks for clarifying @ricardoV94. What is next step from here. Do we need approval for additional maintainers or can the PR be merged now?

ricardoV94 commented 3 weeks ago

@carsten-j we would fix the tests elsewhere, then update this PR on top of the fixes, and if the tests are passing we can merge, unless any of the reviewers blocks it / requests more changes. You don't need to do anything :)

zaxtax commented 2 weeks ago

Looks good. Once the tests pass, I think it's good to merge

ricardoV94 commented 6 days ago

@carsten-j tests are no longer failing in main. You can rebase/merge into your branch

review-notebook-app[bot] commented 4 days ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

carsten-j commented 4 days ago

@ricardoV94 I have rebased the laplace branch but it looks like someone needs to approve Github worksflows.

zaxtax commented 4 days ago

Looks like there are still a few failing tests, but once those pass this is probably good to merge

carsten-j commented 4 days ago

@zaxtax failing test has been fixed. Can you approve the waiting workflow?

carsten-j commented 4 days ago

@zaxtax, all tests passed. Are you also able to merge the PR? Thanks.

twiecki commented 4 days ago

Congrats @carsten-j, this is a big one!

carsten-j commented 4 days ago

Thank you @twiecki. Really happy to contribute and thanks to all those that helped. After the summer I will try to work on documentation for building and running locally. I took me some time to figure out how this works!

zaxtax commented 3 days ago

Congrats @carsten-j this is really neat!

theorashid commented 3 days ago

Brilliant work @carsten-j . Hope to see you contribute to PyMC again!