Flaky test: `test_component_choice_random`

[michaelosthege]

It failed in this run https://github.com/pymc-devs/pymc/actions/runs/3240831110/jobs/5311952415

With

___________________ TestMixture.test_component_choice_random ___________________

self = <pymc.tests.distributions.test_mixture.TestMixture object at 0x7fb12dd6a2f0>

    def test_component_choice_random(self):
        """Test that mixture choices change over evaluations"""
        with Model() as m:
            weights = [0.5, 0.5]
            components = [Normal.dist(-10, 0.01), Normal.dist(10, 0.01)]
            mix = Mixture.dist(weights, components)
        draws = draw(mix, draws=10)
        # Probability of coming from same component 10 times is 0.5**10
>       assert np.unique(draws > 0).size == 2
E       assert 1 == 2
E        +  where 1 = array([False]).size
E        +    where array([False]) = <function unique at 0x7fb14babd360>(array([-10.01041454, -10.00804946, -10.02323384, -10.01368128,\n        -9.99331916,  -9.9980023 ,  -9.98098861, -10.00484836,\n        -9.993316  , -10.00568352]) > 0)
E        +      where <function unique at 0x7fb14babd360> = np.unique

pymc/tests/distributions/test_mixture.py:338: AssertionError

[bherwerth]

I ran this test multiple times, each time printing the draws. The tests succeeded, but the values of draws differ for each run. This makes me think that the test is not seeded, although the test class inherits from SeededTest. Maybe this point is worth investigating and perhaps related to the discussion in #6211 .

If the seed is indeed not fixed, this test is expected to fail with probability 2/1024 (cf. the comment in the code about the same component being chosen 10 times with probability 0.5**10; the test fails if either of the two components is chosen 10 times).

We could lower the probability of the current test failing to practically zero by choosing a larger number of draws. Looking at #6211, perhaps we could also test more comprehensively by using a hypothesis test that each component is chosen with the expected probability. If doing that, though, I don't think increasing the draws will help against the test failing at random.

[mattiadg]

This can be made deterministic like I did with #6220. Just pass the random_seed parameter to draw using get_random_state of SeededTest. However, I don't understand if the idea of the test is exactly not to be deterministic.

If you want to make it deterministic, I can also add it to my PR.

[ricardoV94]

Yes we can seed it

[bherwerth]

If you want to make it deterministic, I can also add it to my PR.

@mattiadg , I created a draft PR, only noticed now that you suggested to add the change to your PR. I've also changed the assertion to test for the expected binomial. If you this this makes sense, I can keep the PR, otherwise I'll withdraw it.

I agree get_random_state makes the tests deterministic. I'd prefer if it did not return the same random state as for the other test cases of the same class. By re-using the same random state, the conditions under which one test case is run can change when changes are made to other test cases -- a minor point, though, and not in scope of this issue it seems.

[mattiadg]

Sure, keep it, you are also adding more checks than me. Il lun 17 ott 2022, 22:01 bherwerth ***@***.***> ha scritto:

…

If you want to make it deterministic, I can also add it to my PR. @mattiadg <https://github.com/mattiadg> , I created a draft PR, only noticed now that you suggested to add the change to your PR. I've also changed the assertion to test for the expected binomial. If you this this makes sense, I can keep the PR, otherwise I'll withdraw it. I agree get_random_state makes the tests deterministic. I'd prefer if it did not return the same random state as for the other test cases of the same class. By re-using the same random state, the conditions under which one test case is run can change when changes are made to other test cases -- a minor point, though, and not in scope of this issue it seems. — Reply to this email directly, view it on GitHub <#6210 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7LDIWHFLT3A4GJQQKL4W3WDWWDDANCNFSM6AAAAAARECBII4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bherwerth]

Thanks @mattiadg . I'll wait for your PR to be merged so I can rebase on your changes.