Make slice sampler sample from 1D conditionals as it should #2446
Conversation
In the previous implementation it would sample jointly from non-scalar variables, and hang for when the size is high (due to low probability to get a joint sample within the slice in high-D).
|
It would be ideal to also add a test for sampling from a high-dimensional variable. |
|
I'd also guess you'll need to update |
|
@junpenglao |
|
The first step is to check whether the test passes locally. |
pymc3/step_methods/slicer.py
Outdated
| @@ -34,8 +34,8 @@ def __init__(self, vars=None, w=1., tune=True, model=None, **kwargs): | |||
| self.model = modelcontext(model) | |||
| self.w = w | |||
| self.tune = tune | |||
| self.w_sum = 0 | |||
| self.n_tunes = 0 | |||
| # self.w_sum = 0 #probably we don't need it now | |||
There was a problem hiding this comment.
i'd rather you just delete this (and line 48) -- tests should fail if they are needed.
| q = np.copy(q0) # TODO: find out if we need this | ||
| ql = np.copy(q0) # l for left boundary | ||
| qr = np.copy(q0) # r for right boudary | ||
| for i in range(len(q0)): |
There was a problem hiding this comment.
Feel free to ignore, but this loop might be cleaner using
for i, (point, width) in enumerate(zip(q, self.w)):
| # breaking markovianness. Can we do it regardless of self.tune?(@madanh) | ||
| self.w[i] = self.w[i] * (self.n_tunes / (self.n_tunes + 1)) +\ | ||
| (qr[i] - ql[i]) / (self.n_tunes + 1) # same as before | ||
| # unobvious and important: return qr and ql to the same point |
| (qr[i] - ql[i]) / (self.n_tunes + 1) # same as before | ||
| # unobvious and important: return qr and ql to the same point | ||
| qr[i] = q[i] | ||
| ql[i] = q[i] | ||
| if self.tune: |
There was a problem hiding this comment.
can remove this second if self.tune, right?
There was a problem hiding this comment.
In principle, yes. Might also leave it there for clarity. I'm sure that performance effects are atomic-scale.
There was a problem hiding this comment.
my objection is that it makes reading it a little funny -- I had to go back and see what the condition had been above.
There was a problem hiding this comment.
yes, but if it's not there then we will increment the tune counter even when not tuning, which is logically wrong and could detract somebody who's reading the code even more!
There was a problem hiding this comment.
ah shoot! you're absolutely right. i was looking at the diff, and it made it look like you were doing
if self.tune:
some_logic
if self.tune:
self.n_tunes += 1
Carry on!
|
tested locally with a simple model below, yes the samples are much more accurate. with pm.Model():
mu = pm.Normal('mu', 0., 10., shape=(100,))
trace = pm.sample(step=pm.Slice(mu)) |
|
A good place to start on local testing is to just run the the three flags ( Once that passes, try running the whole file with (note I dropped the to run the whole test suite (which takes 10s of minutes). |
|
but it might be that I'm not testing the exact PR I'm sending, because I didn't clone the repo locally and testing whatever I had when I added the change. Anyway I'm out for investigation. |
|
Yes, that's an expected failure -- we have those tests since doing rigorous statistical analysis is not really in line with automated testing, so it flags if the sampling method changes at all (this is called a "regression test", which is confusing in the machine learning world :) ). You should be able to change the first key/value pair in |
|
@ColCarroll OK, travis also fails on this test. I was thinkng - if it's an expected failure, should we remove it, should I raise an issue? |
|
@ColCarroll Oh, sorry, I did not get that I have to change the "master sample" in the test itself before now! Will look into it. |
|
@junpenglao @ColCarroll Important update: I will have no spare time next days, so if minimum requirements are satisfied, please consider merging, as I'm not going to add tests or anything. |
|
@madanh No problem. Thank you for the contribution! |
|
Thanks! |
|
Thanks for the package! Cheers :) |
…s#2446) * Make Slice sampler sample from 1D conditionals In the previous implementation it would sample jointly from non-scalar variables, and hang for when the size is high (due to low probability to get a joint sample within the slice in high-D). * slicer.py Fix broken indentation due to copypaste * Apply autopep8 * Delete a superfluous commented line * Update the master sample for Slice in test_step.py
Previous implementation would sample from non-scalar variables (size>1) jointly. Judging from code this was not intended. When sampling from high-dimensional variables it would hang due to small probability of getting the joint sample right when sampling uniformly from a hypercube enclosing the slice.
This PR fixes this by sampling from 1D components of multidimensional variables one at a time.
Admittedly not-ideal since it only returns the last sample, while all the intermediate samples are also valid, effectively performing thinning, but at least it does not hang.
Done while discussing #2189