Flaky test: `pymc/tests/step_methods/hmc/test_nuts.py::TestNUTSNormal::test_kstest`

[mattiadg]

It failed in this pipeline with error:

================================== FAILURES ===================================
_________________________ TestNUTSNormal.test_kstest __________________________

self = <pymc.tests.step_methods.hmc.test_nuts.TestNUTSNormal object at 0x0000022B4908E310>

    def test_kstest(self):
        for varname, cdf in self.cdfs.items():
            samples = self.samples[varname]
            if samples.ndim == 1:
                t, p = stats.kstest(samples[:: self.ks_thin], cdf=cdf)
                assert self.alpha < p
            elif samples.ndim == 2:
                pvals = []
                for samples_, cdf_ in zip(samples.T, cdf):
                    t, p = stats.kstest(samples_[:: self.ks_thin], cdf=cdf_)
                    pvals.append(p)
                t, p = stats.combine_pvalues(pvals)
>               assert self.alpha < p
E               AssertionError

pymc\tests\sampler_fixtures.py:58: AssertionError
---------------------------- Captured stdout setup ----------------------------
\u2588\r\r |----------------------------------------| 0.00% [0/22000 00:00<? Sampling 2 chains, 0 divergences]\r\r |------------------------------------| 0.00% [1/22000 00:00<00:00 Sampling 2 chains, 0 divergences]\r\r |------------------------------------| 0.01% [2/22000 00:00<00:00 Sampling 2 chains, 0 divergences]\r\r |------------------------------------| 0.01% [3/22000 00:00<00:00 Sampling 2 chains, 0 divergences]\r\r |------------------------------------| 0.02% [4/22000 00:00<00:00 Sampling 2 chains, 0 divergences]\r\r |------------------------------------| 0.02% [5/22000 00:00<00:00 Sampling 2 chains, 0 divergences]\r\r |\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588\u2588| 100.00% [22000/22000 00:17<00:00 Sampling 2 chains, 0 divergences]\r
---------------------------- Captured stderr setup ----------------------------
Initializing NUTS using jitter+adapt_diag...
Multiprocess sampling (2 chains in 2 jobs)
NUTS: [a]
Sampling 2 chains for 1_000 tune and 10_000 draw iterations (2_000 + 20_000 draws total) took 27 seconds.
----------------------------- Captured log setup ------------------------------
INFO     pymc:sampling.py:2479 Initializing NUTS using jitter+adapt_diag...
INFO     pymc:sampling.py:606 Multiprocess sampling (2 chains in 2 jobs)
INFO     pymc:sampling.py:244 NUTS: [a]
INFO     pymc:sampling.py:678 Sampling 2 chains for 1_000 tune and 10_000 draw iterations (2_000 + 20_000 draws total) took 27 seconds.

[aseyboldt]

The way the test is written with a ks test and alpha=0.001 we would expect one in every 1000 tests to fail if everything is fine.
I just executed the tests on the latest master of pymc and aesara with a larger number of draws to make sure there isn't something that's broken with the sampler, so I don't think this is something to worry about.
If we want to avoid this in the future, we probably should set a seed so that it stays reproducible...

[mattiadg]

We have seeded other flaky tests because it's not nice that the pipeline fails for reasons unrelated to the changes in a PR. I can do this. Il gio 27 ott 2022, 01:27 Adrian Seyboldt ***@***.***> ha scritto:

…

The way the test is written with a ks test and alpha=0.001 we would expect one in every 1000 tests to fail if everything is fine. I just executed the tests on the latest master of pymc and aesara with a larger number of draws to make sure there isn't something that's broken with the sampler, so I don't think this is something to worry about. If we want to avoid this in the future, we probably should set a seed so that it stays reproducible... — Reply to this email directly, view it on GitHub <#6246 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7LDIQT4QSWAUJPXYUWYELWFG45HANCNFSM6AAAAAARPOMJLU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ricardoV94]

We have seeded other flaky tests because it's not nice that the pipeline
fails for reasons unrelated to the changes in a PR.

Although that's true, it's also the case that not seeding increases coverage. It's more likely we would notice something went broken if this test is not seeded.

Now these tests were a subclass of SeededTest (#6234) so I imagine the original intention was for them to be seeded...

[mattiadg]

Let me know what you think is more appropriate. Maybe this kind of tests can be reexecuted (one or more times) when they fail the first time. I am not sure, though, if this can hide other types of problems.

[ricardoV94]

@aseyboldt WDYT?

[aseyboldt]

One in 1000 tests is actually still relatively common with all the automatic runs we are doing, especially since there are a couple of those tests, and either one could fail independently of the others. If we are used to test failures here, I'm a bit afraid this could lead to us ignoring it if they do fail legitimately. So I guess we could either decrease alpha, and make the tests less likely to fail, but also less sensitive, or we make sure they are actually seeded. I think I prefer making them seeded, that's just overall more predictable.
By the way, this really is I think a case where Frequentist stats does exactly what we want, and Bayesian stats doesn't :-)

[ricardoV94]

Let's go with seeding then ;)