search

XanaduAI / thewalrus

A library for the calculation of hafnians, Hermite polynomials and Gaussian boson sampling.
https://the-walrus.readthedocs.io
Apache License 2.0
101 stars 55 forks source link

Add pytest randomly #205

Closed thisac closed 3 years ago

thisac commented 3 years ago

Context: Some tests are failing randomly (specifically when adding new tests causing the order of the tests to change) due to a combination of them being random and that the seed that is set at the top of each file isn't actually resetting between tests at all (since all files are loaded prior to running any tests).

Description of the Change: pytest-randomly is added as a dependency for the test suite, and is used for all CI tests as well as in the makefile. The seed is set to 41 everywhere. Also, the unnecessary np.random.seeds are removed from the test-files to avoid confusion.

Benefits: pytest-randomly provides 2 significant benefits:

  1. The random seed is reset between each test.

  2. The tests are shuffled, providing an extra layer of testing by making sure that no tests are dependent on the order by which they run.

Possible Drawbacks: Since this changes the way the tests are run, some tests may fail further down the line. This should be considered good though, since it means that we will discover things that aren't working properly. Also, since the seed it set via the CI config files, it might be a bit confusing for some to know that there in fact is a seed set and where it is set (at least for those unfamiliar with pytest-randomly).

Related GitHub Issues: None

co9olguy commented 3 years ago

Sounds like a useful library!

codecov[bot] commented 3 years ago

Codecov Report

Merging #205 into master will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #205   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines         1131      1131           
=========================================
  Hits          1131      1131

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b651eda...5c4e0a7. Read the comment docs.

nquesada commented 3 years ago

Thanks @thisac . I wonder if we should leave the np.random.seed(137) inside the files so that when I (or someone else) runs them locally, they still get deterministic results.

thisac commented 3 years ago

Thanks @nquesada! That makes sense, I'll re-add the np.random.seed to the top of each file. :+1:

thisac commented 3 years ago

@nquesada I just added an np.random.seed(137) to conftest.py, which will always be imported first, so the seed will always be set when running files.

This will behave exactly the same as having a seed at the top of each file, but will not be the same as when using pytest-randomly which resets the seed before each test. Also, if pytest-randomly is installed, the seed will be randomized before each test either way, if the flag --randomly-dont-reset-seed is not set (even when seemingly only running vanilla pytest e.g. pytest thewalrus/tests).