-
-
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
Implement non-blocked sampling for array based samplers. #587
Conversation
Note that this only samples in a non-blocked way on separate RVs. If a RV is multidimensional it will still update those in a blocked way. I have some experimental code that also applies the sampler to each dimension individually. This might be useful as the pymc3 style of creating models is with a vector based syntax rather than for-loops. |
@jsalvatier Just found this: Could this allow for an alternative implementation? Would still have to loop over |
@@ -9,10 +11,44 @@ | |||
|
|||
|
|||
class ArrayStep(object): | |||
def __init__(self, vars, fs, allvars=False): | |||
def __new__(cls, *args, **kwargs): |
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.
I don't really like using new on something like this, seems confusing and complicated. Is there something simpler we could do?
Maybe we could just have a function, elemwise_sampler(vars, sampler, args, kwargs) or something?
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.
I agree that it's adding quite a bit of complexity. The other alternative I saw was #450 and having separate classes. Having a special elemwise_sampler
function seems similar.
I reconsidered this approach though as I think the interface and UX is more important than the implementation.
I also tried to have the element wise iteration over dimensions/RVs be done inside of sample()
but it e.g. didn't work with Metropolis
because scaling
was still the original/blocked dimensionality.
I'm not sure what other way there is to get a simple blocked=False
kwarg without a large refactoring.
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.
@jsalvatier Do you have another idea for implementing this that has the same UX? I don't think it's ideal but personally can't think of a better way.
@twiecki "Could this allow for an alternative implementation? Would still have to loop over vars in ArrayStep.step() though, right?" I don't quite get what you're asking. |
I'm gonna go ahead and merge this soon since there haven't been any alternative ideas and I think it just improves the status quo. |
Implement non-blocked sampling for array based samplers.
Before, all our samplers operated in the joint space (i.e. blocked) by default. This often leads to bad convergence for metropolis or slice.
This adds functionality to set
blocked=False
when instantiating any sampler which will instead create a step-method for each RV and append them to aCompoundStep
.Samplers like metropolis and slice default to not being blocked (as was the case in pymc2 and is what people assume). HMC and NUTS however default to being blocked as they are specifically designed to operate in the joint space.
Unfortunately because of all the configurations
ArrayStep
could be called there's quite some code to determine the configuration it was called in but it's fully backwards compatible that way.This supersedes #450 which took a different approach.