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

Use the new marginal sampler for latent gaussian models in the GP notebook #398

Merged
merged 1 commit into from
Dec 12, 2022
Merged

Use the new marginal sampler for latent gaussian models in the GP notebook #398

merged 1 commit into from
Dec 12, 2022

Conversation

juanitorduz
Copy link
Contributor

@juanitorduz juanitorduz commented Nov 15, 2022

Closes #238

Aim: Use the data from https://blackjax-devs.github.io/blackjax/examples/GP_EllipticalSliceSampler.html to illustrate the implementation introduced in #247

For now, I have a running notebook that runs the example. I still wanted to push this branch and create a draft PR for feedback. What is missing (IMO):

General

  • Add text explaining the method and the results. I will do a very complete (verbose?) explanation for a newbie like me. We can trim the text to make it more succinct.
  • Convert notebook to md (jupytext?)

Details

Any feedback is welcome :)

@juanitorduz juanitorduz marked this pull request as draft November 15, 2022 15:13
@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #398 (4004742) into main (28167ad) will increase coverage by 0.12%.
The diff coverage is n/a.

❗ Current head 4004742 differs from pull request most recent head 36f9e3b. Consider uploading reports for the commit 36f9e3b to get more accurate results

@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
+ Coverage   89.86%   89.99%   +0.12%     
==========================================
  Files          45       47       +2     
  Lines        2161     2188      +27     
==========================================
+ Hits         1942     1969      +27     
  Misses        219      219              
Impacted Files Coverage Δ
blackjax/__init__.py 100.00% <0.00%> (ø)
blackjax/base.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rlouf
Copy link
Member

rlouf commented Nov 17, 2022

All sounds good! And yes, you will need to use jupytext to convert the notebook to md.

@rlouf rlouf added the documentation Improvements or additions to documentation label Nov 20, 2022
@AdrienCorenflos
Copy link
Contributor

@juanitorduz sorry I didn't see this

At the moment I am setting the arbitrary value of alpha=0.5 when defining the kernel. Should we use a calibration step as illustrated in https://github.com/blackjax-devs/blackjax/blob/main/examples/LogisticRegressionWithLatentGaussianSampler.md ?

alpha is a parameter in the prior, I am not sure what you mean by "calibration step". The calibration step in the notebook mentioned is for the delta parameter which corresponds (in a loose sense) to the step-size of MALA and other similar algos. Are you trying to learn alpha?

@juanitorduz
Copy link
Contributor Author

Ahh! Thanks for the input! I was indeed confused! Now it makes sense. I will add the text and work on the notebook next week.
Thanks for the input!

@AdrienCorenflos
Copy link
Contributor

no pb

@AdrienCorenflos
Copy link
Contributor

On a side note, good choice of example. The original paper considers GP classification and it shows that their method (mGrad, which is the one I implemented here) dominates the elliptical slice sampler (Ellipt):

image

@juanitorduz juanitorduz marked this pull request as ready for review December 2, 2022 10:15
@juanitorduz
Copy link
Contributor Author

I think I have a first initial complete iteration ready for review :)

examples/GP_Marginal.md Outdated Show resolved Hide resolved
examples/GP_Marginal.md Outdated Show resolved Hide resolved
examples/GP_Marginal.md Outdated Show resolved Hide resolved
examples/GP_Marginal.md Outdated Show resolved Hide resolved
examples/GP_Marginal.md Outdated Show resolved Hide resolved
examples/GP_Marginal.md Outdated Show resolved Hide resolved
examples/GP_Marginal.md Outdated Show resolved Hide resolved
```

## Run Sampler

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have a quick example of how the sampler works, like in the Elliptical Slice Sampling notebook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! I want to do this... unfortunately I have not been able to access the paper (I am trying through old access from my university). Is there a pre-print available?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OMG 🤦 I can not believe I did not find this! I think I was keen to get the published version. Anyway, let me give a read and try to write this section myself, and then I'll collect feedback. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, this is the "executive summary" of the method I have in the paper I am currently writing.
image

Copy link
Member

@rlouf rlouf left a comment

Choose a reason for hiding this comment

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

It looks great! I modified the titles to match the notebook with the Elliptical Slice sampler (we may merge them eventually). It would also be nice to have a short explanation of how the sampler works if you feel like writing it.

@juanitorduz juanitorduz requested review from rlouf and AdrienCorenflos and removed request for rlouf and AdrienCorenflos December 9, 2022 13:21
@juanitorduz
Copy link
Contributor Author

@rlouf @AdrienCorenflos In 687f295 I tried to express in my words how I understand the sampler works. I have a math background, which is far from this area so I feel it is at the moment out of my reach to have a very detailed and transparent description of the details behind the sampler (if you want to talk about geometric analysis I feel a bit better prepared hehe). Hence, feel free to modify / add or erase parts that you believe are confusing or incorrect.

@juanitorduz juanitorduz removed the request for review from rlouf December 9, 2022 13:27
@juanitorduz juanitorduz requested review from AdrienCorenflos and rlouf and removed request for AdrienCorenflos December 9, 2022 13:27
@AdrienCorenflos
Copy link
Contributor

@rlouf @AdrienCorenflos In 687f295 I tried to express in my words how I understand the sampler works. I have a math background, which is far from this area so I feel it is at the moment out of my reach to have a very detailed and transparent description of the details behind the sampler (if you want to talk about geometric analysis I feel a bit better prepared hehe). Hence, feel free to modify / add or erase parts that you believe are confusing or incorrect.

I made some changes to the explanation of the algo.
https://github.com/AdrienCorenflos/blackjax/tree/patch-2
@rlouf if you could check and merge?

@rlouf
Copy link
Member

rlouf commented Dec 10, 2022

I'll take a look early next week, thanks.

@rlouf
Copy link
Member

rlouf commented Dec 12, 2022

I added the changes suggested by @AdrienCorenflos (and added him as co-author) and rebased the commits. It looks good to me, can you take a look one last time before I merge?

rlouf
rlouf previously approved these changes Dec 12, 2022
@juanitorduz
Copy link
Contributor Author

Thank you for the feedback and corrections @AdrienCorenflos ! LGTM.

@rlouf
Copy link
Member

rlouf commented Dec 12, 2022

Merging now, great work everyone!

@rlouf rlouf merged commit 2e38b77 into blackjax-devs:main Dec 12, 2022
Co-authored-by: Adrien Corenflos <adrien.corenflos@aalto.fi>
@juanitorduz juanitorduz deleted the gp_marginal_nb_238 branch December 12, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the new marginal sampler for latent gaussian models in the GP notebook
3 participants