<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jun 10, 2018 at 11:15 PM, Robert Kern <span dir="ltr"><<a href="mailto:robert.kern@gmail.com" target="_blank">robert.kern@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span class="">On Sun, Jun 10, 2018 at 8:11 PM Ralf Gommers <<a href="mailto:ralf.gommers@gmail.com" target="_blank">ralf.gommers@gmail.com</a>> wrote:<br>><br>> On Sun, Jun 10, 2018 at 5:57 PM, Robert Kern <<a href="mailto:robert.kern@gmail.com" target="_blank">robert.kern@gmail.com</a>> wrote:<br>>><br>>> On Sun, Jun 10, 2018 at 5:47 PM Ralf Gommers <<a href="mailto:ralf.gommers@gmail.com" target="_blank">ralf.gommers@gmail.com</a>> wrote:<br>>> ><br>>> > On Sun, Jun 3, 2018 at 9:23 PM, Warren Weckesser <<a href="mailto:warren.weckesser@gmail.com" target="_blank">warren.weckesser@gmail.com</a>> wrote:<br>>><br>>> >> I suspect many of the tests will be easy to update, so fixing 300 or so tests does not seem like a monumental task.<br>>> ><br>>> > It's all not monumental, but it adds up quickly. In addition to changing tests, one will also need compatibility code when supporting multiple numpy versions (e.g. scipy when get a copy of RandomStable in scipy/_lib/_numpy_compat.py).<br>>> ><br>>> > A quick count of just np.random.seed occurrences with ``$ grep -roh --include \*.py np.random.seed . | wc -w`` for some packages:<br>>> > numpy: 77<br>>> > scipy: 462<br>>> > matplotlib: 204<br>>> > statsmodels: 461<br>>> > pymc3: 36<br>>> > scikit-image: 63<br>>> > scikit-learn: 69<br>>> > keras: 46<br>>> > pytorch: 0<br>>> > tensorflow: 368<br>>> > astropy: 24<br>>> ><br>>> > And note, these are *not* incorrect/broken usages, this is code that works and has done so for years.<br>>><br>>> Yes, some of them are incorrect and broken. Failure can be difficult to detect. This module from keras is particularly problematic:<br>>><br>>>   <a href="https://github.com/keras-team/keras-preprocessing/blob/master/keras_preprocessing/image.py" target="_blank">https://github.com/keras-team/<wbr>keras-preprocessing/blob/<wbr>master/keras_preprocessing/<wbr>image.py</a><br>><br>> You have to appreciate that we're not all thinking at lightning speed and in the same direction. If there is a difficult to detect problem, it may be useful to give a brief code example (or even line of reasoning) of how this actually breaks something.<br><br></span><div>Ahem. Sorry. That wasn't the code I was thinking of. It's merely hazardous, not broken by itself. However, if you used any of the `seed=` arguments that are helpfully(?) provided, you are almost certainly writing broken code. If you must use np.random.seed() to get reproducibility, you need to call it exactly once at the start of your code (or maybe once for each process) and let it ride.</div><div><br></div><div>This is the impossible-to-use-correctly code that I was thinking of, which got partially fixed after I pointed out the problem.</div><div><br></div><div>  <a href="https://github.com/keras-team/keras/pull/8325/files" target="_blank">https://github.com/keras-team/<wbr>keras/pull/8325/files</a><br></div><div><br></div><div>The intention of this code is to shuffle two same-length sequences in the same way. So now if I write my code well to call np.random.seed() once at the start of my program, this function comes along and obliterates that with a fixed seed just so it can reuse the seed again to replicate the shuffle.</div></div></blockquote><div><br></div><div>Yes, that's a big no-no. There are situations conceivable where a library has to set a seed, but I think the right pattern in that case would be something like<br></div><div><br></div><div>old_state = np.random.get_state()</div><div>np.random.seed(some_int)</div><div>do_stuff()</div><div>np.random.set_state(**old._state)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>Puzzlingly, the root sin of unconditionally and unavoidably reseeding for some of these functions is still there even though I showed how and why to avoid it. This is one reason why I was skeptical that merely documenting RandomState or StableRandom to only be used for unit tests would work. :-)</div></div></blockquote><div><br></div><div>Well, no matter what we do, I'm sure that there'll be lots of people who will still get it wrong:)</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><span class="">>> > Conclusion: the current proposal will cause work for the vast majority of libraries that depends on numpy. The total amount of that work will certainly not be counted in person-days/weeks, and more likely in years than months. So I'm not convinced yet that the current proposal is the best way forward.<br>>><br>>> The mere usage of np.random.seed() doesn't imply that these packages actually require stream-compatibility. Some might, for sure, like where they are used in the unit tests, but that's not what you counted. At best, these numbers just mean that we can't eliminate np.random.seed() in a new system right away.<br>><br>> Well, mere usage has been called an antipattern (also on your behalf), plus for scipy over half of the usages do give test failures (Warren's quick test). So I'd say that counting usages is a decent proxy for the work that has to be done.<br><br></span>Sure. But with my new proposal, we don't have to change it (as much as I'd like to!). I'll draft up a PR to modify my NEP accordingly.<br></div></div></blockquote><div><br></div><div>Sounds good!</div><div><br></div><div>Cheers,<br></div><div>Ralf</div><div><br></div></div></div></div>