Implemented CategoricalGibbsMetropolis, optimized BinaryGibbsMetropolis. #1439
Conversation
|
I think this is fine based on what @twiecki said. On 11 Oct 2016 4:54 PM, "Mehmet Ozan Kabak" [email protected]
|
|
There should also be one or more unit tests for the step method. |
| candidate += 1 | ||
| return candidate | ||
|
|
||
| def softmax(x): |
There was a problem hiding this comment.
Why not use theano.tensor.nnet.softmax?
There was a problem hiding this comment.
Can you use this version for more numerical stability: http://www.johndcook.com/blog/2010/01/20/how-to-compute-the-soft-maximum/ ?
There was a problem hiding this comment.
I am already making use of that trick. See the - np.max(x) term there? This multivariate form is a generalization of John Cook's bivariate expression.
| @@ -283,6 +295,122 @@ def competence(var): | |||
| return Competence.IDEAL | |||
| return Competence.INCOMPATIBLE | |||
|
|
|||
| class CategoricalGibbsMetropolis(ArrayStep): | |||
| """Metropolis-Hastings optimized for categorical variables. This step | |||
There was a problem hiding this comment.
Also add info that this is doing "Gibbs" -- i.e. updating each dimension one at a time, rather than blocked.
There was a problem hiding this comment.
Done. See new commit.
|
Agreed on tests. @wanderer2 Why is the binary step method more efficient? Or rather, why can't the Categorical be also as efficient? |
|
OK, I will send one more commit with:
@twiecki: Theano's About the binary step method: It is more efficient because it proposes new samples by simple binary negation, whereas the categorical step method would incur the overhead of going through random sampling only to propose the same value. |
Also Added extra comments requested by PyMC3 devs, and fixed a minor bug in the proportional proposal utilized by CategoricalGibbsMetropolis.
|
Hi guys. This latest commit has the additions you guys requested. Cheers! |
|
This looks good. Can you check the example notebooks whether they need updating? |
|
I have been working on updating the example notebooks -- a bunch have drifted out of date. I'll push what I have later tonight. |
|
Looking at the notebooks after @ColCarroll's updates, the only notebook that uses I say we keep that notebook as is (for now) and update it later on. It will provide an example use for As a bonus, I have been working on a new notebook that demonstrates how All in all, this PR is ready to get merged, in my opinion. Any objections? |
|
Thanks for the contribution! |
I took up on the discussion we had in Issue #1431, and implemented the
CategoricalGibbsMetropolissampler @twiecki and I contemplated there. I also optimized the implementation ofBinaryGibbsMetropolis, and deprecatedElemwiseCategorical.