Multivariate floating point precision fix#2470
Multivariate floating point precision fix#2470ColCarroll merged 14 commits intopymc-devs:masterfrom ericmjl:multivariate_fix
Conversation
|
Just saw a test fail, I think I know what's going on. Let me fix that. |
|
Okay, locally tests seem like they're passing. $ py.test pymc3/tests/test_distributions_random.py
============================ test session starts ============================
platform linux -- Python 3.6.0, pytest-3.0.5, py-1.4.32, pluggy-0.4.0
rootdir: /home/ericmjl/github/software/pymc3, inifile: setup.cfg
collected 214 items
pymc3/tests/test_distributions_random.py ....................................
.............................................................................
.............................................................................
.....................xss
============ 211 passed, 2 skipped, 1 xfailed in 253.00 seconds =============
$ py.test pymc3/tests/test_distributions.py
============================ test session starts ============================
platform linux -- Python 3.6.0, pytest-3.0.5, py-1.4.32, pluggy-0.4.0
rootdir: /home/ericmjl/github/software/pymc3, inifile: setup.cfg
collected 95 items
pymc3/tests/test_distributions.py ....................................x...x..
...xxx...x.....................................x....
=================== 88 passed, 7 xfailed in 55.32 seconds ===================I'm feeling a bit more confident now that the tests on Travis should pass. |
|
@ColCarroll @junpenglao all tests pass!!!!!! |
ColCarroll
left a comment
ColCarroll
left a comment
There was a problem hiding this comment.
looks good to merge now -- had a few small comments if you have a second. excited to try this out and see if it fixes my problems!
pymc3/distributions/multivariate.py
Outdated
| self.mode = tt.cast(tround(self.mean), 'int32') | ||
|
|
||
| def _random(self, n, p, size=None): | ||
| # Set float type to float64 for numpy. |
There was a problem hiding this comment.
can you add a comment with a link to the numpy issue -- that'll make it clearer why this cast exists, and if/when we can remove it
pymc3/distributions/multivariate.py
Outdated
| raise ValueError('Outcome probabilities must be 1- or 2-dimensional ' | ||
| '(supplied `p` has {} dimensions)'.format(p.ndim)) | ||
|
|
||
| return randnum |
There was a problem hiding this comment.
should the return value get cast back to the original data type? honest question -- maybe @kyleabeauchamp has an opinion?
There was a problem hiding this comment.
No idea. But X is discrete while p is continuous, so presumably if it worked previously so it should continue working with the current patch.
There was a problem hiding this comment.
I think it's a good idea. I will implement that fix.
pymc3/distributions/multivariate.py
Outdated
| size = None | ||
|
|
||
| if p.ndim == 1: | ||
| p = p / (p.sum()) |
There was a problem hiding this comment.
nit -- extra parens around p.sum(...), both here and below
|
@ColCarroll I ran the two relevant tests locally and found that they pass. I'm confident it'll pass on Travis, but as usual, let's wait it out. |
|
@ColCarroll @junpenglao @kyleabeauchamp I think one of the tests timed out, is that correct? Otherwise, everything else has passed. |
|
Yeah, one is a flaky test, one looks like coveralls threw a 5xx error. I restarted both. |
|
Thanks for fixing this! |
|
Yay!!!! Thanks @ColCarroll! |
3 participants
This is a bug fix for
distributions.multivariate's Multinomial errors related to floating point precision. This will very likely be helpful for computation runs that involve the GPU, and involve many classes with small floating point numbers.More information can be found here: #2469