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

Importance sampler #692

Merged
merged 7 commits into from
Jul 1, 2022
Merged

Importance sampler #692

merged 7 commits into from
Jul 1, 2022

Conversation

michaeldeistler
Copy link
Contributor

@michaeldeistler michaeldeistler commented May 24, 2022

This implements an ImportanceSamplingPosterior.

This posterior behaves as follows:

  • log_prob(): estimates the normalization constant of the potential_fn with importance sampling and then returns the potential_fn(theta) - log(norm_constant).
  • sample(): Depending on method, it either returns samples from the proposal and their corresponding importance weights or it samples with SIR (default)

Also, I realized that the "sir" init in MCMC does not actually perform SIR. SIR resamples with weights p(theta|x) / q(theta), but our weights were simply p(theta|x). I renamed this init to resample. resample is also the default. Setting init_strategy='sir' will perform "actual" SIR.

@michaeldeistler michaeldeistler changed the title SIR sampler as separate sampler Importance sampler Jun 21, 2022
@michaeldeistler michaeldeistler force-pushed the sirsampler branch 2 times, most recently from 3252189 to fdcaaf3 Compare June 22, 2022 12:49
@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2022

Codecov Report

Merging #692 (ddc43a0) into main (3405f8b) will decrease coverage by 0.82%.
The diff coverage is 54.21%.

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

@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
- Coverage   77.30%   76.47%   -0.83%     
==========================================
  Files          76       78       +2     
  Lines        5869     5930      +61     
==========================================
- Hits         4537     4535       -2     
- Misses       1332     1395      +63     
Flag Coverage Δ
unittests 76.47% <54.21%> (-0.83%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sbi/inference/posteriors/rejection_posterior.py 82.05% <ø> (-0.45%) ⬇️
sbi/samplers/vi/__init__.py 100.00% <ø> (ø)
sbi/samplers/vi/vi_utils.py 75.97% <ø> (-0.71%) ⬇️
sbi/samplers/importance/sir.py 23.07% <23.07%> (ø)
sbi/inference/posteriors/importance_posterior.py 29.03% <29.03%> (ø)
sbi/inference/posteriors/mcmc_posterior.py 68.79% <60.00%> (-0.78%) ⬇️
sbi/samplers/mcmc/init_strategy.py 83.78% <60.00%> (-4.10%) ⬇️
sbi/samplers/importance/importance_sampling.py 86.20% <86.20%> (ø)
sbi/inference/__init__.py 100.00% <100.00%> (ø)
sbi/inference/posteriors/__init__.py 100.00% <100.00%> (ø)
... and 3 more

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 3405f8b...bcc6a09. Read the comment docs.

@michaeldeistler michaeldeistler requested a review from janfb June 22, 2022 13:17
@michaeldeistler michaeldeistler force-pushed the sirsampler branch 2 times, most recently from 68bf102 to d86e78b Compare June 24, 2022 10:02
Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

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

Great PR!
see comments below.

sbi/inference/posteriors/importance_posterior.py Outdated Show resolved Hide resolved
sbi/inference/posteriors/importance_posterior.py Outdated Show resolved Hide resolved
sbi/inference/posteriors/importance_posterior.py Outdated Show resolved Hide resolved
sbi/inference/posteriors/mcmc_posterior.py Show resolved Hide resolved
sbi/samplers/mcmc/init_strategy.py Show resolved Hide resolved
sbi/samplers/mcmc/init_strategy.py Show resolved Hide resolved
sbi/samplers/vi/vi_quality_control.py Show resolved Hide resolved
tests/linearGaussian_snle_test.py Outdated Show resolved Hide resolved
@janfb
Copy link
Contributor

janfb commented Jun 26, 2022

Great to have the additional sampling methods as Posteriors as well!
Once this one and the arViz PR #542 (I am on it) are both merged we should set up a benchmark to see how they compare and to see which hyperparameters to use!

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

Successfully merging this pull request may close these issues.

3 participants