Repository navigation
TST: Do not use random seed in doctest #11566
Description
See #10644 (review) from @tupui
We did this (#10644) in SciPy and the only difference is that we removed all seeding from examples. This is to enforce best practices. Using small seeds is bad for the entropy of the generator and also common values like 0, 42, 12345 end up in every code which leads to bias in results. Seeing should only be used for testing.
Here I am proposing to remove the seed and just allow variation in the results to pass the doctests (in SciPy I can add # random or # may vary, I am not sure if this is standard or not. Maybe this is your # doctest: +FLOAT_CMP?).
I can do a PR on this branch if you want some help. Also, I would suggest to add somewhere in the doc a paragraph about seeding. See what we did for example https://scipy.github.io/devdocs/tutorial/stats.html#random-number-generation.
Activity
pllim commentedon Apr 14, 2021
p.s. Yes, @tupui , your help would be greatly appreciated if you want to give it. 😸
tupui commentedon Apr 15, 2021
@pllim I will be happy to propose a PR 😃
tupui commentedon Apr 16, 2021
FYI, we are still having lot of discussion on SciPy about how to do this in the best way. I propose to do a PR once we come to a decision on our side. This way I would have better arguments and possibly a better solution for you here.
tupui commentedon Apr 19, 2021
So, we finally came to a solution 😅 In the end we are using
np.random.default_rng()without using# randomor anything else to skip the test. This works because we added an override ofnp.random.default_rng()intonp.random.default_rng(SEED).So the question is, do you want to have this seeding override in order to still have deterministic examples (and be able to rely on this for testing), or do you want to just skip the tests by marking with
# randomor something else? In both cases,np.random.default_rng()would be used without seeding, which is what we want to show to users.pllim commentedon Apr 19, 2021
Re: #11566 (comment) -- Thanks for the update, @tupui ! Just so I understand,
np.random.default_rng()is now deterministic? In what release of Numpy will this new behavior first be introduced?We don't have
# randomdirective in our doctest, so if things work the same without it, I don't see the point of starting to use it now.tupui commentedon Apr 19, 2021
Sorry, my explanation was misleading.
np.random.default_rng()is not deterministic and will not be. But in SciPy, before we execute the doctests, we patchnp.random.default_rng()tonp.random.default_rng(some_seed_value). This way, we have a deterministic behaviour.pllim commentedon Apr 19, 2021
Ah, as in using
.. testsetup::(ref)? That might do. I have done it in my packages before to make things less confusing in the doc.tupui commentedon Apr 19, 2021
A bit yes. Except that here we re-write a given line before running the test. Of course, you could just define the
rngin this block and just tell the user to what it corresponds (like you may already import NumPy or Pandas and havenp, pdavailable without the need to import it in the docstrings).For reference, in SciPy you can see the logic in
scipy.tools.refguide_check._run_doctestswhich is usingscipy._lib._utils._fixed_default_rngpllim commentedon Apr 19, 2021
Ah, so I am guessing you are talking about the new feature introduced in scipy/scipy#13863 that is milestoned to
scipy1.7.0.Currently, we have:
astropy/setup.cfg
Line 66 in 8615fc8
I am not sure if we can take advantage of this until our
scipyminversion is 1.7.0. While in APE 18, we adopted NEP 29, its policy on when to bumpscipyis unclear; maybe @Cadair , @bsipocz , @eteq , or @astrofrog can comment.tupui commentedon Apr 19, 2021
In any case I would not recommend to import this from SciPy. Everything in private module
_*can break without notice. We only guarantee the stability of the public API. So would you chose this solution, I would have to implement it here to ensure it does not break.pllim commentedon Apr 19, 2021
I can see how this solution would be nice but it also introduce more "magic" in the doctest and might make things harder to debug in the future. On the other hand, using
testsetupis more explicit but might introduce a lot of duplication across the docs.If indeed we want to use what you implemented for
scipy, perhaps it should be inpytest-doctestplusinstead...? cc @saimntl;dr -- I am not sure and I should not be making this decision on my own. Will need discussions. Thanks for your info!
tupui commentedon May 4, 2021
FYI, for another reference, I did this for scikit-image here scikit-image/scikit-image#5357
tupui commentedon Jun 23, 2021
Any update here?
pllim commentedon Jun 23, 2021
Update: It is on the agenda for our "infrastructure tag-ups" but we haven't gotten around to it yet. Thanks for your continued patience! 😬
tupui commentedon Jun 23, 2021
Ok thanks for the update! No problem, just let me know 😉
eteq commentedon Jun 28, 2021
One clarification here, @tupui (coming out of the insfrastructure telecon @pllim mentioned): in some of our applications of random number generating (especially in downstream coordinated packages), we use the random number generator to produce a random data set that undergoes further processing where the answer does matter. For example, in the
specutilspackage, some of the examples produce a fake spectral line, which then gets a few processing steps done on it and then gets reduced to, say, the width or total flux of the line. So that's a case where we use the seed not really as a source of randomness or entropy, but rather just as a way to produce a vaguely realistic-looking dataset. In that case# may vary/skipping doesn't seem like the right approach. Is this sort of thing in either the scipy or scikit-image examples, or is this somewhat unique to how we are using the RNGs?tupui commentedon Jun 28, 2021
@eteq in SciPy we introduced a way to replace the line
>>> rng = np.random.default_rng()to include a seed. This was a compromise to help the transition and make people happy with doctests still being tests. My POV is that you should avoid this. RNG is RNG.If you need fake data which are realistic, then store this somewhere. Downside is that it might increase the size, but would also be faster as nothing would need to be generated. You can also store the data elsewhere. On scikit-image, they have some pre-generated data in some tests (you can see they regenerated some examples in the issue I linked).