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

fix errors in pymc sampler implementaton #1129

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Conversation

felixp8
Copy link
Contributor

@felixp8 felixp8 commented Apr 8, 2024

What does this implement/fix? Explain your changes

Two errors in PyMCSampler fixed here:

  • check for whether gradient tracking needed in line 141 of sbi.samplers.mcmc.pymc_wrapper looked for matching step class instead of the string value actually used and expected
  • pymc.Model() incorrectly set up using a prior distribution and additional likelihood term which does not result in the desired distribution. Now using pymc.DensityDist which directly defines the distribution using given potential function
    With these changes test_c2st_pymc_sampler_on_Gaussian passes locally for all sampling methods.

Does this close any currently open issues?

Fixes #1127

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)

@felixp8 felixp8 linked an issue Apr 8, 2024 that may be closed by this pull request
@janfb janfb marked this pull request as ready for review April 8, 2024 10:51
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! It seems that this fixes all the problems? (see https://github.com/sbi-dev/sbi/actions/runs/8597285263/job/23555649100#step:7:3794)

@felixp8 can you please rebase on main to include the recent fixes to the MCMC kwargs?

@janfb janfb added the bug Something isn't working label Apr 8, 2024
@felixp8
Copy link
Contributor Author

felixp8 commented Apr 8, 2024

yup will try to do by eod

@janfb janfb mentioned this pull request Apr 8, 2024
@janfb janfb self-assigned this Apr 8, 2024
@janfb
Copy link
Contributor

janfb commented Apr 8, 2024

After rebasing on main:

With your fixes, the slice and nuts samplers are working fine on the more difficult test. However, hmc is still failing and fluctuates with c2sts around 0.6.

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.26%. Comparing base (7be2115) to head (b8d4ab5).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1129      +/-   ##
==========================================
- Coverage   85.45%   85.26%   -0.19%     
==========================================
  Files          90       89       -1     
  Lines        6612     6616       +4     
==========================================
- Hits         5650     5641       -9     
- Misses        962      975      +13     
Flag Coverage Δ
unittests 85.26% <100.00%> (-0.19%) ⬇️

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

Files Coverage Δ
sbi/samplers/mcmc/pymc_wrapper.py 88.40% <100.00%> (ø)

@felixp8
Copy link
Contributor Author

felixp8 commented Apr 8, 2024

darn okay I'm somewhat sure model log-probs are correct now but it's possible also that the gradient is not correct. Don't think I'll have time to carefully go through the pymc internals for that until later this week

@janfb
Copy link
Contributor

janfb commented Apr 8, 2024

darn okay I'm somewhat sure model log-probs are correct now but it's possible also that the gradient is not correct. Don't think I'll have time to carefully go through the pymc internals for that until later this week

OK, but given that nuts is working fine it's likely that the gradients are correct, no?

No worries, maybe I will find time to have a look, or we just wait. 👍

@janfb
Copy link
Contributor

janfb commented Apr 8, 2024

actually, I ran the CD workflow with slow tests and they are passing now (we got lucky?).
I will run it again to see how far we can push it.

@janfb
Copy link
Contributor

janfb commented Apr 9, 2024

Tests are still passing, and I checked the posterior visually as well - all seems fine!

Thanks for solving this @felixp8 ! 👏

@janfb janfb merged commit 46db263 into main Apr 9, 2024
10 of 11 checks passed
@janfb janfb deleted the 1127-pymc-samplers-fail branch April 9, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyMC samplers failing on simple Gaussian example.
2 participants