Repository navigation
BUG: internal call fix in random.choice - #2823
Conversation
There was a problem hiding this comment.
Technically this just checks that m.choice calls some self random function that affects the output; really we want to test that it only calls self random functions. But obviously it catches this particular bug, so this is finicky. I think the ideal tests for this kind of thing though look like:
for i in xrange(3):
np.random.seed(i)
assert_equal(RandomState(1234).choice(10, ...), [0, 3, 5, 2, ..., 7])
i.e. scrambling the global random state around while checking that a fixed seed continues to produce the same fixed output, and where this output is specified explicitly so that we can compare this version of numpy's .choice method to all the other versions we've shipped.
This kind of test is relevant for all the random functions though, arguably a separate project.
|
Oh yes and the fix itself is obviously correct, and @certik this is RC. |
An random.random call from within mtrand was done by a call to np.random.random instead of inside the class. This can possibly lead to non-deterministic results after seeding.
|
OK, changed it to your version... can't hurt to do it the nice way, some day someone might copy the design. |
|
Yeah you're right; I was just trying to be nice :-) I would hit the green button but already turned off my computer for the
|
|
(Whose job is it to regenerate mtrand.c?)
|
|
oh, I have regenerated it here (cython version 0.17.1), but maybe better if someone else does it, certik has done it with the other PR I think, so if it makes things easier, I can remove that again. |
|
I think this is fine, just wanted to make sure that I didn't have to make sure to do it when merging into master. |
BUG: internal call fix in random.choice
|
Backported in #2827. |
3 participants
An random.random call from within mtrand was done by a call to np.random.random instead of inside the class. This can possibly lead to non-deterministic results after seeding.
Sorry that I noticed this only after mtrand was recreated already for this function... @njsmith this is the extremely simple fix, I guess @certik should put this into 1.7. as well? I included a regression test that failed before and double checked that there is no further occurrence of this at the time.