-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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 bug in Slice sampler and speedup test_step.py
#5816
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5816 +/- ##
==========================================
+ Coverage 89.36% 89.38% +0.01%
==========================================
Files 74 74
Lines 13757 13769 +12
==========================================
+ Hits 12294 12307 +13
+ Misses 1463 1462 -1
|
It seems like this is how the sampler used to behave until commit b988ba9 changed |
Wow that means the Slice sampler was broken since version 3.2. |
Should we backport the fix to V3? |
I'm not sure if it's worth it, but an exception telling users to upgrade to v4 should be easy enough. |
It's the same amount of work to add an exception or change the 2 lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I should mention something in the Release Notes |
4430d78
to
de94c8c
Compare
1. The previous interval was of the form `[q0 - uniform(0, self.w), q0 + self.w]` which is not symmetric around q0 in expectation, breaking detailed balance. 2. The left/right bound values used for updating earlier variables were only set to the accepted values during tuning. This might have made bug 1 worse.
Avoid recreation of RaveledArrays in inner loops and repeated indexing of `self.w`
The number of draws was set too high to accommodate the worst / buggy samplers (see pymc-devs#5815)
Added Release note |
Closes #5815
I changed the logic when creating the initial interval according to MacKay (2005, page 375) [pdf].
Before
After
I think the previous was causing the proposals to be systematically biased towards higher values (right).
Also made sure that we set the boundaries of previous variables to the accepted values, which probably by mistake was only being done during tuning.
If someone more familiar with these can confirm my changes to
Slice
make sense that would be great.The sampler now converges in the multivariate example in #5815, and does an increasingly better job with more samples.
Also did some other minor changes, see individual commits for more details.