Bug Fix for 2909 #2979

springcoil · 2018-05-17T19:15:00Z

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 · 2018-05-17T19:30:37Z

pymc3/tests/test_distributions_random.py

+            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,


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

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

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 · 2018-05-17T19:56:55Z

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 · 2018-05-17T20:00:55Z

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 · 2018-05-17T20:02:36Z

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 · 2018-05-17T20:06:58Z

pymc3/distributions/distribution.py

@@ -355,7 +357,9 @@ def _draw_value(param, point=None, givens=None):
        if point and hasattr(param, 'model') and param.name in point:
            return point[param.name]
        elif hasattr(param, 'random') and param.random is not None:
-            return param.random(point=point, size=None)
+            return param.random(point=point, size=None).mean(axis=0)
+        elif hasattr(param, 'random') and param.random is not None and size is not None:


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

Ok I'll remove that branch.

ColCarroll · 2018-05-17T20:08:19Z

pymc3/distributions/distribution.py

@@ -355,7 +357,9 @@ def _draw_value(param, point=None, givens=None):
        if point and hasattr(param, 'model') and param.name in point:
            return point[param.name]
        elif hasattr(param, 'random') and param.random is not None:
-            return param.random(point=point, size=None)
+            return param.random(point=point, size=None).mean(axis=0)


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.

Let me try removing the mean :)

springcoil · 2018-05-17T20:15:36Z

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 · 2018-05-17T20:19:03Z

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 · 2018-05-17T20:47:27Z

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

Fixing up the test and implementation Adding other draw_values Small test fix

springcoil · 2018-05-18T05:35:02Z

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

ColCarroll · 2018-05-18T11:40:07Z

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 · 2018-05-18T11:57:30Z

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 · 2018-05-18T20:52:39Z

#2982 -- release notes done

springcoil requested a review from ColCarroll May 17, 2018 19:15

springcoil changed the title ~~BUG: Attempt to fix 2909~~ WIP: Attempt to fix 2909 May 17, 2018

springcoil added bug WIP labels May 17, 2018

springcoil modified the milestones: 3.5, 3.4 May 17, 2018

ColCarroll reviewed May 17, 2018

View reviewed changes

springcoil force-pushed the 2909-attempt-at-fix branch from 1f8e094 to ca8a997 Compare May 17, 2018 20:14

springcoil force-pushed the 2909-attempt-at-fix branch from ca8a997 to 017f36b Compare May 17, 2018 20:46

BUG: Attempt to fix 2909

b53d413

Fixing up the test and implementation Adding other draw_values Small test fix

springcoil force-pushed the 2909-attempt-at-fix branch from 3cdf561 to b53d413 Compare May 17, 2018 21:49

springcoil changed the title ~~WIP: Attempt to fix 2909~~ Bug Fix for 2909 May 18, 2018

springcoil removed the WIP label May 18, 2018

twiecki assigned ColCarroll May 18, 2018

twiecki mentioned this pull request May 18, 2018

Fix for issue #2909 #2946

Closed

ColCarroll merged commit ac8fe5a into master May 18, 2018

junpenglao deleted the 2909-attempt-at-fix branch May 18, 2018 12:51

springcoil mentioned this pull request May 18, 2018

sample_ppc does not respect the size of Mixture likelihoods #2954

Closed

ColCarroll mentioned this pull request May 18, 2018

Add sample_generative function #2983

Merged

canyon289 mentioned this pull request Jul 14, 2018

Fix categorical random shape #3060

Merged

Uh oh!

Bug Fix for 2909 #2979

Bug Fix for 2909 #2979

Uh oh!

Conversation

springcoil commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ColCarroll May 17, 2018

Choose a reason for hiding this comment

Uh oh!

springcoil May 17, 2018

Choose a reason for hiding this comment

Uh oh!

ColCarroll May 17, 2018

Choose a reason for hiding this comment

Uh oh!

springcoil commented May 17, 2018 via email

Uh oh!

springcoil commented May 17, 2018

Uh oh!

springcoil commented May 17, 2018

Uh oh!

ColCarroll May 17, 2018

Choose a reason for hiding this comment

Uh oh!

springcoil May 17, 2018

Choose a reason for hiding this comment

Uh oh!

ColCarroll May 17, 2018

Choose a reason for hiding this comment

Uh oh!

springcoil May 17, 2018

Choose a reason for hiding this comment

Uh oh!

springcoil commented May 17, 2018

Uh oh!

ColCarroll commented May 17, 2018

Uh oh!

springcoil commented May 17, 2018

Uh oh!

springcoil commented May 18, 2018

Uh oh!

ColCarroll commented May 18, 2018

Uh oh!

springcoil commented May 18, 2018 via email

Uh oh!

springcoil commented May 18, 2018

Uh oh!

Uh oh!

springcoil commented May 17, 2018 •

edited

Loading