vonMises: Avoid overflow for large kappa #2832
Conversation
|
diverges might comes from funnel in the Real transformed space, looking at the Metropolis might not be very informative. |
| @@ -931,6 +931,7 @@ def test_ex_gaussian(self, value, mu, sigma, nu, logp): | |||
| pt = {'eg': value} | |||
| assert_almost_equal(model.fastlogp(pt), logp, decimal=select_by_precision(float64=6, float32=2), err_msg=str(pt)) | |||
|
|
|||
| @pytest.mark.xfail(condition=(theano.config.floatX == "float32"), reason="Fails on float32") | |||
There was a problem hiding this comment.
Why was this not come up before? If the problem is scipy having overflow?
There was a problem hiding this comment.
Because, before both overflow. Now we can use vonmises with larger values of kappa.
|
Thanks for the feedback, one more thing @junpenglao. The only reason we have now i0 and i1 is to compute the variance when initializing the vonMises distribution. Should be remove them (and not compute the variance) or keep them? |
|
Is the current computation reliable? It is really quite minor and I doubt anybody is using it. So I would agree with removing it if the computation itself is also not very robust. |
pymc3/distributions/special.py
Outdated
| @@ -3,7 +3,7 @@ | |||
| from theano.scalar.basic_scipy import GammaLn, Psi, I0, I1 | |||
| from theano import scalar | |||
|
|
|||
| __all__ = ['gammaln', 'multigammaln', 'psi', 'i0', 'i1', 'log_i0'] | |||
| __all__ = ['gammaln', 'multigammaln', 'psi', 'log_i0'] | |||
There was a problem hiding this comment.
should also remove the import from theano above.
log_i0instead of log(i0) for computing vonMises' logp.Note: I still get some warnings about diverges (specially for kappa ~< 1). Despite these warnings the samples looks right compared against SciPy or sampling using Metropolis. I will see if I can fix this in a future PR.