Bug Fix for 2909

[springcoil]

I tried to have a go at extending #2946 and adding in a test.

I think there's a chance these tests will be flaky, and I discovered some flakiness locally.

This is an attempt to fix #2909

[ColCarroll]

heh, now this is surprising to me that these values are all above 0.5. Any guess why?

[springcoil]

I think it varies a bit by the run. That's just one run of it. Is this wrong?

[ColCarroll]

It is random so anything can happen! I am expecting this test case to be equivalent to

samples = []
for _ in range(10000):
    a = np.random.uniform(0, 1, 10)
    samples.append(np.random.binomial(n=1, p=a))
np.array(samples).mean(axis=0)

and it is very rare to see any values in the resulting array be outside of (0.48, 0.52), much less all 10 values (and less again all 10 being greater than 0.5).

I would guess there is still a subtle bug, or else I am thinking of this wrong.

[springcoil]

Ahh interesting. Yeah I get what you mean finally!!! So still some subtle bug is here. Is it related to the pm.Uniform being passed to pm.Binomial - or some bug in my implementation?

…

On Thu, May 17, 2018 at 8:39 PM, Colin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pymc3/tests/test_distributions_random.py <#2979 (comment)>: > @@ -97,6 +97,15 @@ def test_random_sample_returns_nd_array(self): assert isinstance(mu, np.ndarray) assert isinstance(tau, np.ndarray) + def test_random_sample_returns_correctly(self): + # Based on what we discovered in #GH2909 + with pm.Model(): + a = pm.Uniform('a', lower=0, upper=1, shape=10) + b = pm.Binomial('b', n=1, p=a, shape=10) + array_of_randoms = b.random(size=10000).mean(axis=0) + npt.assert_allclose(array_of_randoms, [0.5338, 0.5443, 0.5313, 0.5392, 0.5372, 0.5435, 0.5287, 0.5432, It is random so anything can happen! I am expecting this test case to be equivalent to samples = [] for _ in range(10000): a = np.random.uniform(0, 1, 10) samples.append(np.random.binomial(n=1, p=a)) np.array(samples).mean(axis=0) and it is very rare to see any values in the resulting array be outside of (0.48, 0.52), much less all 10 values (and less again all 10 being greater than 0.5). I would guess there is still a subtle bug, or else I am thinking of this wrong. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2979 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8DiKntjbeEbOKvoDPbjCu9reLqOO3uks5tzdHbgaJpZM4UDoBJ> .

-- Peadar Coyle Skype: springcoilarch www.twitter.com/springcoil peadarcoyle.wordpress.com

[springcoil]

When I run it locally.

I get

Out[10]:
array([0.3491, 0.3488, 0.3407, 0.3508, 0.3531, 0.3508, 0.3471, 0.3474,
       0.3461, 0.3523])

In [11]: b.random(size=10000).mean(axis=0)
Out[11]:
array([0.7232, 0.7302, 0.7265, 0.7278, 0.729 , 0.7369, 0.7249, 0.7228,
       0.728 , 0.7335])

In [12]: b.random(size=10000).mean(axis=0)
Out[12]:
array([0.4958, 0.4996, 0.4979, 0.4898, 0.4962, 0.4933, 0.4972, 0.4999,
       0.4938, 0.5012])

In [13]: b.random(size=100000).mean(axis=0)
Out[13]:
array([0.59457, 0.59537, 0.59586, 0.59437, 0.59508, 0.59411, 0.59349,
       0.59711, 0.59586, 0.59473])

So my test implementation seems to be incorrect. However, does the output look correct? Or still some subtle bug?

[springcoil]

Originally @ColCarroll said

with pm.Model() as model:
    a = pm.Uniform('a', lower=0, upper=1, shape=10)
    b = pm.Binomial('b', n=1, p=a, shape=10)
    
b.random(size=10000).mean(axis=0)

# array([0.7022, 0.0073, 0.9857, 0.5378, 0.9821, 0.7176, 0.0905, 0.2513, 0.5835, 0.0521])
I would have expected this mean to be close to 0.5 for each element.

``` - is this expected behaviour incorrect now? 

[ColCarroll]

this branch will never get hit (since the above condition will always hit)

[springcoil]

Ok I'll remove that branch.

[ColCarroll]

this mean here is causing the strangeness. In your test, it is just drawing 10 values for a, taking the mean (which is likely reasonably close to 0.5), and then using that value for all 10k draws of b.

[springcoil]

Let me try removing the mean :)

[springcoil]

Ok I've made a few of those changes. I'll wait to see what happens when it runs on travis and then seed the results with those numbers.

[ColCarroll]

Sounds good. I suspect you will also need to update every single distribution that calls draw_values. Maybe try uniform and binomial to see if it makes the test case run sensibly? The uniform call is here.

[springcoil]

Seems to make the test case run sensibly locally. I'll let this run and then hardcode in the results.

[springcoil]

Ok it seems that this works to some extent. Do we need other tests? @ColCarroll

[ColCarroll]

Wow. This looks great! Going to merge since tests pass and it fixes my counterexample, but I will kick the tires on it today.

We should also keep an eye on http://pandas.pydata.org/speed/pymc3/ , but I think this is perfect. Thanks for tackling a tough issue, @springcoil !

Can you also add this to the release notes?

[springcoil]

Ok I'll add this to the release notes later

…

On Fri, 18 May 2018, 12:40 pm Colin, ***@***.***> wrote: Merged #2979 <#2979>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2979 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8DiApWj8HzSBLHPuZpm1gHVHbG21SKks5tzrMvgaJpZM4UDoBJ> .

[springcoil]

#2982 -- release notes done