made test_dres_convergence less likely to fail

[zemanj]

Fixes #1931

Changes made in this Pull Request:

• If MDAnalysisTests.analysis.test_encore.TestEncore.test_dres_convergence fails, a RuntimeWarning is thrown and the test is repeated 10 times.
• Commented out printf(...) in CStochasticProximityEmbedding()

PR Checklist

• Tests?
• Docs?
• CHANGELOG updated?
• Issue raised/referenced?

[coveralls]

Coverage decreased (-0.02%) to 89.793% when pulling 5945a1f on zemanj:dres_convergence into 9675f92 on MDAnalysis:develop.

[zemanj]

Simply repeating the test 10 times on failure is certainly not an ideal solution, it simply decreases the likelihood of random failures. Maybe someone else comes up with a better idea.
The decreased coverage seems majorly BS to me.

[kain88-de]

To the solution is to switch out the RNG into something we can reliably seed ourselves. This can be done by porting the c extensions into cython. Then using the numpy rng can always use the same seed. That will make the tests reproducible.

[zemanj]

@kain88-de Porting the C extensions to Cython sounds quite cumbersome and probably overkill to me...
Personally, I'd always prefer a C code interfaced with Cython over a "pure" Cython implementation.

However, regarding the option of setting a seed externally: Even though the implementation of rand() is compiler-specific (or at least depends on libc), it might still be ok to set a fixed seed externally (only for testing, of course), since it is rather unlikely to hit one of the seeds that cause test failure (regardless of the rand() implementation). Adding that option should be a lot easier to accomplish than porting the entire extension to Cython.
Nevertheless, in case of a fixed seed, the functionality is only ever tested for that particular seed. Not sure whether that's a good idea.

[kain88-de]

Nevertheless, in case of a fixed seed, the functionality is only ever tested for that particular seed. Not sure whether that's a good idea.

You can rerun with different seeds if you like. But that is overkill IMHO. For tests using fixed seeds is OK because we want to reliable setup the same environment for a test.

Adding that option should be a lot easier to accomplish than porting the entire extension to Cython.

Well yeah maybe. But random numbers make all of this a bit more complicated when one wants to do them right. I prefer to give users a consistent interface to any RNG within mdanalysis and the easiest way to achieve this is to go with the numpy rng's. This also allows to run the analysis with multiple processes and give each process it's own correctly seeded RNG.

[zemanj]

@kain88-de Sounds convincing. I agree that having a consistent PRNG interface throughout the code should be the way to go. The current state of the stochasticproxembed extension is not satisfactory in that respect, and even less when taking into account both reproducibility and random number quality.

How would you like proceed with this? I'd suggest using this PR as a band-aid and open a separate issue regarding PRNG interfaces.

[kain88-de]

make the seed an argument to the function. Then we can provide one for testing. This will make the tests more reproducible instead of just rerunning them and hoping it will work out.

[richardjgowers]

@kain88-de what you're proposing is the better solution for this, but this PR fixes the problem with false negatives on PRs nicely, we should merge

[richardjgowers]

70b9e9f

@zemanj looks like maybe your email isn't configured right?