Integrate PyMC samplers and clean up unsued MCMC sampler #1053

famura · 2024-03-20T10:53:38Z

What does this implement/fix? Explain your changes

Integrate the mighty MCMC samplers from PyMC.
Remove unused MCMC samplers.

Does this close any currently open issues?

Fixes #987 and partially #986

Any relevant code examples, logs, error output, etc?

...

Any other comments?

Currently the MCMC class sbi.samplers.sample.mcmc.mcmc.py is only used in the test

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

I have read and understood the contribution
guidelines
I agree with re-licensing my contribution from AGPLv3 to Apache-2.0.
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works
I have reported how long the new tests run and potentially marked them
with pytest.mark.slow.
New and existing unit tests pass locally with my changes
I performed linting and formatting as described in the contribution
guidelines
I rebased on main (or there are no conflicts with main)

…nto 987-pymc-interface

felixp8 · 2024-03-20T16:26:06Z

@famura it should be in decent shape for you to check over now

famura

great progress

sbi/inference/posteriors/mcmc_posterior.py

sbi/samplers/mcmc/__init__.py

sbi/samplers/mcmc/pymc_utils.py

tests/mcmc_test.py

sbi/samplers/mcmc/pymc_utils.py

famura · 2024-03-22T18:40:06Z

This will have a conflict with PR #1107 since that PR changes some tests that we are deleting here. However, I think that can be solved no matter which PR we merge first. I guess this one is more ready.

@felixp8

felixp8

all looks good to me, but I'm biased :) would maybe be best to get another review also?

janfb

Great work @famura @felixp8 👏
Added a couple of comments and questions.

sbi/inference/posteriors/mcmc_posterior.py

sbi/samplers/mcmc/pymc_wrapper.py

# Conflicts: # tests/mcmc_slice_pyro/test_slice.py # tests/mcmc_test.py # tests/posterior_sampler_test.py

famura · 2024-03-28T10:08:46Z

@felixp8 please have a look at the state after the merge

felixp8 · 2024-04-02T14:17:47Z

@famura all looks good to me! any thoughts from y'all on whether we should expose the multiprocessing start method (i.e. "spawn" vs "fork") as a parameter for Pyro and PyMC samplers as Jan mentions here? I don't really know what difference it makes tbh

janfb

Thanks for the updates!
I added a couple of suggestion for the thin doc strings. Sorry for the causing confusion here..

sbi/inference/posteriors/mcmc_posterior.py

sbi/samplers/mcmc/slice_numpy.py

janfb · 2024-04-03T08:32:58Z

@famura all looks good to me! any thoughts from y'all on whether we should expose the multiprocessing start method (i.e. "spawn" vs "fork") as a parameter for Pyro and PyMC samplers as Jan mentions here? I don't really know what difference it makes tbh

Forking can be 20x faster for multiprocessing but works only on unix systems:

The fork method is only supported on POSIX-based systems like Linux and macOS (not Windows), 
whereas the spawn start method is supported on all platforms. A major difference between the two
start methods is speed. It is generally considered that forking a process is faster than spawning a process

https://superfastpython.com/fork-faster-than-spawn/#:~:text=The%20fork%20method%20is%20only,faster%20than%20spawning%20a%20process.

Thus, I think it would be good to expose it so that users can use fork when they want.

famura · 2024-04-03T08:34:15Z

... and it doesn't work with CUDA afaik

felixp8 · 2024-04-03T08:48:47Z

makes sense, happy to quickly patch that in unless you're already on it @famura

Co-authored-by: Jan <[email protected]>

famura · 2024-04-03T09:25:09Z

no, please go ahead @felixp8

felixp8 · 2024-04-03T10:51:50Z

added support for setting mp_context="fork", will debug test failures later this afternoon

codecov · 2024-04-03T18:24:56Z

Codecov Report

Attention: Patch coverage is 86.60714% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 76.89%. Comparing base (4c2f71e) to head (707605c).
Report is 41 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1053      +/-   ##
==========================================
+ Coverage   76.37%   76.89%   +0.51%     
==========================================
  Files          84       89       +5     
  Lines        6507     6591      +84     
==========================================
+ Hits         4970     5068      +98     
+ Misses       1537     1523      -14

Flag	Coverage Δ
unittests	`76.89% <86.60%> (+0.51%)`	⬆️

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

Files	Coverage Δ
sbi/samplers/mcmc/__init__.py	`100.00% <100.00%> (ø)`
sbi/samplers/mcmc/slice_numpy.py	`91.98% <100.00%> (+0.06%)`	⬆️
sbi/inference/posteriors/mcmc_posterior.py	`85.85% <89.74%> (+0.29%)`	⬆️
sbi/samplers/mcmc/pymc_wrapper.py	`84.28% <84.28%> (ø)`

... and 13 files with indirect coverage changes

famura added 2 commits March 20, 2024 11:07

Deleted unused MCMC class, using Pyro's leads to MP errors currently

a5550e9

First try on fixing the pickling error

894b9bc

famura added enhancement New feature or request hackathon labels Mar 20, 2024

famura assigned famura and felixp8 Mar 20, 2024

famura linked an issue Mar 20, 2024 that may be closed by this pull request

Add interface to PyMC MCMC samplers #987

Closed

famura and others added 6 commits March 20, 2024 11:54

Merge remote-tracking branch 'origin/main' into 987-pymc-interface

c539c10

Moved test class

1ed672f

Test are running

b95be9f

Removed the Pyro-based slice sampler

0290799

add draft pymc interface

2d61fa9

Merge branch '987-pymc-interface' of https://github.com/sbi-dev/sbi i…

e015bba

…nto 987-pymc-interface

fix dependency typo

c934a71

famura commented Mar 21, 2024

View reviewed changes

sbi/samplers/mcmc/pymc_utils.py Outdated Show resolved Hide resolved

famura and others added 13 commits March 21, 2024 09:58

Minor edits

0c65731

Deleted empty file

9ca4902

improved docstrings, attempt to fix ruff and pyright

f9cee22

improve test

4012de3

@famura improve tests

dfc4387

Minor test input rework

dfe8085

Docs

26001fe

Thinning has fallen out of fashion, ask PyMC

51a38d6

Unified warmup_steps default to 200

bb6459d

More asserts in the test

aca454b

Bug fix

df15735

Improved test_api_posterior_sampler_set

0574f7a

Merge remote-tracking branch 'origin/main' into 987-pymc-interface

101cfcd

This was referenced Mar 22, 2024

Better default arguments for MCMC, SNLE, SNPE. #910

Closed

Uniform and better MCMC params for the tests #1107

Merged

felixp8 mentioned this pull request Mar 22, 2024

Problem with the number of chains in MCMC_Posterior with method = 'nuts' #1077

Closed

update docstring for num_chains

b6d7134

felixp8 approved these changes Mar 25, 2024

View reviewed changes

famura requested a review from michaeldeistler March 25, 2024 09:56

janfb reviewed Mar 25, 2024

View reviewed changes

famura added 2 commits March 28, 2024 10:40

Merge remote-tracking branch 'origin/main' into 987-pymc-interface

e7c39cc

# Conflicts: # tests/mcmc_slice_pyro/test_slice.py # tests/mcmc_test.py # tests/posterior_sampler_test.py

Fixes after merge

bc39ca5

famura added 3 commits March 28, 2024 11:11

Doc fix

0bfe4bc

Changed tests parameters

b47b443

Doc updates from review

37f3633

janfb reviewed Apr 3, 2024

View reviewed changes

felixp8 and others added 2 commits April 3, 2024 11:14

apply suggested change to thinning docstring

3bb7663

Co-authored-by: Jan <[email protected]>

apply suggested change to thinning docstring [2]

7feb7c4

Co-authored-by: Jan <[email protected]>

expose mc_context argument

bf69d18

fix assumption of default thin value

707605c

janfb approved these changes Apr 3, 2024

View reviewed changes

janfb merged commit 6e0e98a into main Apr 3, 2024
5 checks passed

janfb deleted the 987-pymc-interface branch April 3, 2024 19:33

janfb mentioned this pull request Apr 9, 2024

Add new MCMC samplers #908

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate PyMC samplers and clean up unsued MCMC sampler #1053

Integrate PyMC samplers and clean up unsued MCMC sampler #1053

famura commented Mar 20, 2024 •

edited

Loading

felixp8 commented Mar 20, 2024

famura left a comment

famura commented Mar 22, 2024

felixp8 left a comment

janfb left a comment

famura commented Mar 28, 2024

felixp8 commented Apr 2, 2024

janfb left a comment

janfb commented Apr 3, 2024

famura commented Apr 3, 2024

felixp8 commented Apr 3, 2024

famura commented Apr 3, 2024

felixp8 commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

Integrate PyMC samplers and clean up unsued MCMC sampler #1053

Integrate PyMC samplers and clean up unsued MCMC sampler #1053

Conversation

famura commented Mar 20, 2024 • edited Loading

What does this implement/fix? Explain your changes

Does this close any currently open issues?

Any relevant code examples, logs, error output, etc?

Any other comments?

Checklist

felixp8 commented Mar 20, 2024

famura left a comment

Choose a reason for hiding this comment

famura commented Mar 22, 2024

felixp8 left a comment

Choose a reason for hiding this comment

janfb left a comment

Choose a reason for hiding this comment

famura commented Mar 28, 2024

felixp8 commented Apr 2, 2024

janfb left a comment

Choose a reason for hiding this comment

janfb commented Apr 3, 2024

famura commented Apr 3, 2024

felixp8 commented Apr 3, 2024

famura commented Apr 3, 2024

felixp8 commented Apr 3, 2024 • edited Loading

codecov bot commented Apr 3, 2024 • edited Loading

Codecov Report

famura commented Mar 20, 2024 •

edited

Loading

felixp8 commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading