1st commit of reworked DEMetropolis(Z) drop tune fraction notebook.

View / edit / reply to this conversation on ReviewNB

michaelosthege commented on 2023-01-25T22:24:06Z ----------------------------------------------------------------

Typo NUTS

I like the tl;dr idea for notebooks!

gbrunkhorst commented on 2023-01-26T15:07:16Z ----------------------------------------------------------------

Will change

review-notebook-app[bot] commented 1 year ago

View / edit / reply to this conversation on ReviewNB

michaelosthege commented on 2023-01-25T22:24:07Z ----------------------------------------------------------------

in the second formula you can use \cdot for the product

gbrunkhorst commented on 2023-01-26T15:07:40Z ----------------------------------------------------------------

will revise per comment

michaelosthege commented 1 year ago

This is great! Very thorough investigation, and the notebook remains focused too.

@OriolAbril is there a policy about outsourcing some code to a helpers.py file next to the notebook? This one needs quite a few helper functions for which an interested reader could be referred to the source.

@gbrunkhorst should we change the default proposal distribution to NormalProposal as recommended in the original paper?

gbrunkhorst commented 1 year ago

Will change

View entire conversation on ReviewNB

gbrunkhorst commented 1 year ago

will revise per comment

View entire conversation on ReviewNB

OriolAbril commented 1 year ago

@OriolAbril is there a policy about outsourcing some code to a helpers.py file next to the notebook? This one needs quite a few helper functions for which an interested reader could be referred to the source.

cells can be hidden (input only, output only or both, for helper functions 1st or 3rd option work), I would use that, there are already several notebooks using this feature. Reference: https://myst-nb.readthedocs.io/en/latest/render/hiding.html

gbrunkhorst commented 1 year ago

Thanks, @michaelosthege I'm really glad you like the changes.

Regarding helpers.py : I did use input-hide as a cell tag for the helper functions, which turns them into "click to open" cells in jupyterbook. This may show up in the html even though it is hidden in the notebook. I would need to check the html preview page, although I'm not sure how to find it. . . If this feature is working, then I think we have solved the issue of being readable but still including code for the interested reader. More broadly, I would recommend that all matplotlib code is hidden and all pymc code is shown as a rule of thumb. The matplotlib code gets really lengthy and distracting to the student.

Regarding defaults, it seems like pymc should use an unbounded proposal for epsilon. From the [ter Braak paper](https://link.springer.com/article/10.1007/s11222-008-9104-9):

Because of the unbounded support of e in (1), the joint chain is ergodic, each of the N chains converges to the posterior distribution, and at any time after convergence the N chains are independent. The proof is given in the Appendix.

I don't have the math to verify this claim, but it seems like NormalProposal would be more consistent with the paper than UniformProposal. This would apply to both MVMetropolis and MVMetropolis(Z). I would also support changing the default tuning = scaling since lamb inherently scales to the posterior and scaling does not (as mentioned in the notebook). Finally, I have an idea for MVNormalProposal : tuning A (rather than scaling ) by calculating the cov on n tuning steps and feeding cov to A . I can experiment with that separately if there is interest.

michaelosthege commented 1 year ago

I don't have the math to verify this claim, but it seems like NormalProposal would be more consistent with the paper than UniformProposal. This would apply to both MVMetropolis and MVMetropolis(Z). I would also support changing the default tuning = scaling since lamb inherently scales to the posterior and scaling does not (as mentioned in the notebook).

Feel free to open a PR for this in https://github.com/pymc-devs/pymc or let me know if you rather won't!

Finally, I have an idea for MVNormalProposal : tuning A (rather than scaling ) by calculating the cov on n tuning steps and feeding cov to A . I can experiment with that separately if there is interest.

There's a MultivariateNormalProposal but it doesn't have an A parameter? Tuning the covariance matrix of the proposal dist has been explored a lot in literature, and for different algorithms too. Mass matrix tuning for NUTS, pathfinder, DE-MCMC, all these are somewhat related in this regard.

There's definitely room for smarter tuning schemes when it comes to the Metropolis samplers in PyMC.

gbrunkhorst commented 1 year ago

cells can be hidden (input only, output only or both, for helper functions 1st or 3rd option work), I would use that, there are already several notebooks using this feature. Reference: https://myst-nb.readthedocs.io/en/latest/render/hiding.html

Great - I think the cells will be hidden in the html rendering.

Feel free to open a PR for this in https://github.com/pymc-devs/pymc or let me know if you rather won't!

I can do give this a shot.

OriolAbril commented 1 year ago

I would need to check the html preview page, although I'm not sure how to find it. . .

There is no preview for this PR yet, the check is failing. It looks like an issue with the bibtex new reference.

gbrunkhorst commented 1 year ago

I pushed just the updated bibtex file - note it also includes the terBraak 2006 reference, which we will need at some point.

gbrunkhorst commented 1 year ago

@michaelosthege I reworked the other DEMetropolis notebook (efficiency comparison) and pushed it on this same branch. The DEMetropolisZ sampler was consistently more efficient, but the tail of the sampled distribution was a bit small at 50 dimensions. I'll be curious about your thoughts. I really like these samplers for gradient-free inference I'm trying to understand any potential limitations.

michaelosthege commented 1 year ago

The second notebook re-write is as impressive as the first! I left a few minor comments.

The DEMetropolisZ sampler was consistently more efficient, but the tail of the sampled distribution was a bit small at 50 dimensions. I'll be curious about your thoughts. I really like these samplers for gradient-free inference I'm trying to understand any potential limitations.

I'm definitely not an expert on this, but I heard that the concern with Metropolis samplers is that for high-dimensional densities they underestimate the tails. Now at the same time, the DE-MCMC-Z proposal distribution only converges to the posterior over time, and because they start out as one small cluster, this means that their variance just slowly increases over time. Sometimes this can be seen if the tuning settings were bad, and in high-dimensional densities this would (to my understanding) translate to the aforementioned underestimation of the tails.

Surprisingly, sampling was slower using multiple cores rather than one core for both samplers for the same number of total samples.

This might be because the inter-process communication overhead exceeds the cost of sequentially evaluating the logp & gradient on the same CPU core. Also, if your number of logical CPU cores does not exceed the number of chains for DEMetropolis some chains will compute sequentially..

michaelosthege commented 1 year ago

When you resolve these git conflicts make sure to create a backup copy of your branch.

I don't trust the pre-commit stuff with these Markdown files... Once I lost two notebook rewrites to this because pre-commit/I messed up.

gbrunkhorst commented 1 year ago

Thanks for the review @michealosthege I'm really glad that you found the expanded notebook useful. I think the ball is in my court to revise both DEMetropolis notebooks based on comments and kick them back for review by @oriolabril

gbrunkhorst commented 1 year ago

Sorry for the confusion - I was working through changes but had not pushed them yet - both notebooks are now revised and pushed and should be ready.

michaelosthege commented 1 year ago

Sorry for the confusion - I was working through changes but had not pushed them yet - both notebooks are now revised and pushed and should be ready.

You'll need to resolve the git conflicts on the *.md files. Make sure to have a backup copy of the notebooks and let me know if you need assistance

gbrunkhorst commented 1 year ago

Sorry - I'm not sure how to resolve git conflicts. Does it have to do with fetching or rebasing?

On Sun, Feb 12, 2023 at 9:09 AM Michael Osthege @.***> wrote:

Sorry for the confusion - I was working through changes but had not pushed them yet - both notebooks are now revised and pushed and should be ready.

You'll need to resolve the git conflicts on the *.md files. Make sure to have a backup copy of the notebooks and let me know if you need assistance

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

OriolAbril commented 1 year ago

checked out the PR and ran:

git fetch upstream
git rebase upstream/main
pre-commit run --all
git commit -a -m "run pre-commit"

I don't trust the pre-commit stuff with these Markdown files... Once I lost two notebook rewrites to this because pre-commit/I messed up.

as an extra note, this can no longer happen. myst files are generated with pre-commit to make sure they match the notebook, there is no myst -> ipynb flow of information anymore, it is now a pre-commit check that runs, modifies only myst files and always fixes itself (a bit like the black check). On the flip side, when we leave review comments in myst files (for myst syntax or metadata it is the only place they can be reviewed), you can never accept the suggestions or make the changes as is, you need to make those changes in the ipynb files

gbrunkhorst commented 1 year ago

Thanks (both of you) for enabling me to contribute! I couldn’t figure this out without all your hard work. On Feb 15, 2023, at 12:03 AM, Michael Osthege @.***> wrote: @michaelosthege approved this pull request.

Thanks for explaining/fixing, @OriolAbril and thanks again to @gbrunkhorst for the rewriting! I think this is good to merge now

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

pymc-devs / pymc-examples

1st commit of reworked DEMetropolis(Z) drop tune fraction notebook. #509

{Insert Description}

Helpful links