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 features to support ArviZ integration, Rebased #607

Merged
merged 3 commits into from
Aug 10, 2022
Merged

Conversation

janfb
Copy link
Contributor

@janfb janfb commented Jan 25, 2022

Successor of #546 that was rebased onto main to include the new sampling interface. See comments there for reference.

Tasks:

  • add arviz dependencies
  • add arviz test with Pyro MCMC
  • add example notebook
  • feedback
  • rebase and group commits.

Changes:

  • add posterior_sampler attribute to MCMCPosterior that saves, e.g., the Pyro sampler
  • refactor slice samplers: we have the sequential and the vectorized slice samplers with slightly different API. @sethaxen added a wrapper to simplify the calls to the two different slice sampling cases in MCMCPosterior
  • the slice samplers now have a get_samples method just like Pyro samplers to get the samples for all chains.
  • using get_samples we can construct arviz InferenceData objects from all the samplers used in MCMCPosterior
  • Note that we construct InferenceData from samples for the Pyro samplers as well (and not using az.from_pyro(sampler)) because only then we can make sure the samples are not in transformed space.
  • parallel slice sampling MCMC: before we had a separate function that parallelized both the sequential and the vectorized slice samplers. The parallelization is now integrated into the sequential slice sampler. However, it is not (yet) integrated into the vectorized version. Thus, after this PR, the vectorized slice sampler will not run parallelized (I think that OK because it is not clear what the trade-off is between parallelization and vectorization).
  • new option in sample to directly return the arviz object: this is useful as we have all the mcmc parameters and separate chains present in the sample method.

fixes #542

@janfb janfb added the enhancement New feature or request label Jan 25, 2022
@janfb janfb self-assigned this Jan 25, 2022
@sethaxen
Copy link
Contributor

Thanks! @janfb I'm happy to review when ready.

@janfb
Copy link
Contributor Author

janfb commented Jul 14, 2022

I tried to resolve a series of merge conflicts due to API changes in main. In particular, the refactoring of the slice sampling classes conflicts with the slice_sampling_parallized (note the typo :D). I added TODO in line and will mark this as draft for now.

@janfb janfb marked this pull request as draft July 14, 2022 10:36
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2022

Codecov Report

Merging #607 (094625c) into main (d72fc6d) will increase coverage by 0.40%.
The diff coverage is 89.00%.

❗ Current head 094625c differs from pull request most recent head 228a0f3. Consider uploading reports for the commit 228a0f3 to get more accurate results

@@            Coverage Diff             @@
##             main     #607      +/-   ##
==========================================
+ Coverage   73.85%   74.25%   +0.40%     
==========================================
  Files          79       79              
  Lines        5970     6032      +62     
==========================================
+ Hits         4409     4479      +70     
+ Misses       1561     1553       -8     
Flag Coverage Δ
unittests 74.25% <89.00%> (+0.40%) ⬆️

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

Impacted Files Coverage Δ
sbi/samplers/mcmc/__init__.py 100.00% <ø> (ø)
sbi/samplers/mcmc/slice_numpy.py 91.98% <83.33%> (-3.76%) ⬇️
sbi/inference/posteriors/mcmc_posterior.py 82.02% <97.22%> (+14.89%) ⬆️
sbi/inference/posteriors/base_posterior.py 81.08% <100.00%> (+0.25%) ⬆️
sbi/utils/sbiutils.py 78.86% <100.00%> (+0.26%) ⬆️
sbi/utils/potentialutils.py 100.00% <0.00%> (+12.50%) ⬆️

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

@janfb janfb force-pushed the arviz_rebase branch 4 times, most recently from 043a6fe to 8b7c241 Compare August 9, 2022 18:10
@janfb janfb marked this pull request as ready for review August 9, 2022 18:16
@janfb
Copy link
Contributor Author

janfb commented Aug 9, 2022

@sethaxen I finally finished this PR and would be very happy to get your feedback. No problem if do not find the time, just let me know.

@sethaxen
Copy link
Contributor

sethaxen commented Aug 9, 2022

Awesome! I'm heading on vacation tomorrow so will review sometime on or after the 22nd if you haven't merged yet.

Copy link
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

had a quick look, let's discuss in a minute

sbi/inference/posteriors/base_posterior.py Outdated Show resolved Hide resolved
sbi/inference/posteriors/mcmc_posterior.py Outdated Show resolved Hide resolved
sbi/inference/posteriors/mcmc_posterior.py Outdated Show resolved Hide resolved
sbi/inference/posteriors/mcmc_posterior.py Show resolved Hide resolved
sbi/inference/posteriors/mcmc_posterior.py Outdated Show resolved Hide resolved
sbi/inference/posteriors/mcmc_posterior.py Show resolved Hide resolved
sbi/samplers/mcmc/slice_numpy.py Show resolved Hide resolved
sbi/samplers/mcmc/slice_numpy.py Outdated Show resolved Hide resolved
sbi/utils/sbiutils.py Outdated Show resolved Hide resolved
sethaxen and others added 2 commits August 10, 2022 15:53
Accept tuning kwarg to SliceSampler

Add SliceSamplerSerial

Shares same interface as SliceSamplerVectorized

Add posterior_sampler property

Reset sampler after rejection sampling

Set default var_name

Interpret num_samples as per chain

Test SliceSamplerSerial

Test tuning keyword

Test tuning and thin keywords

Test posterior_sampler correctly set

adapt mcmc refactoring and arviz integration to new sampler interface.

Co-authored-by: Seth Axen <seth.axen@gmail.com>
Co-authored-by: janfb <j.f.boelts@gmail.com>

refactor usage and parallelization of slice samplers.
add option to construct InferenceData during sampling.

update test.

isort.

add arviz.

fix get_arviz.

refactor, add get_samples to np slice samplers.

arviz tutorial.
@janfb janfb force-pushed the arviz_rebase branch 2 times, most recently from 5406a9d to 094625c Compare August 10, 2022 14:05
@janfb
Copy link
Contributor Author

janfb commented Aug 10, 2022

@sethaxen I am merging this now to get it into the next release, but I am happy to discuss the changes / improve the arviz tutorial in September (when I will be back). Thanks, and have a good vacation! 🧘

- fix reset of posterior sampler for new x.
- fix docstrings.
@janfb janfb merged commit d7ac218 into main Aug 10, 2022
@janfb janfb deleted the arviz_rebase branch August 10, 2022 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ArviZ integration
4 participants