BUG: Fix random.choice when probability is not C contiguous #13716
Conversation
numpy/random/tests/test_random.py
Outdated
| def test_choice_probas_not_contiguous(self): | ||
| p = np.repeat(np.array([[0.1, 0, 0.3, 0.6, 0]]).T, 3, axis=1) | ||
| random.choice(5, 3, p=p[:, 1]) | ||
|
|
There was a problem hiding this comment.
test should also try np.random.Generator().choice(...) which would also fail.
There was a problem hiding this comment.
... and np.random.multinomial, and np.random.Generator().multinomial
There was a problem hiding this comment.
This is in the wrong file. Should be test_randomstate.py
There was a problem hiding this comment.
And again in test_generator_mt19937.py
numpy/random/mtrand.pyx
Outdated
| p = <np.ndarray>np.PyArray_FROM_OTF(p, np.NPY_DOUBLE, np.NPY_ALIGNED) | ||
| p = <np.ndarray>np.PyArray_FROM_OTF( | ||
| p, np.NPY_DOUBLE, np.NPY_ALIGNED | np.NPY_ARRAY_C_CONTIGUOUS | ||
| ) |
There was a problem hiding this comment.
would be better to change the kahan_sum function rather than forcing a copy here. There are more places that need testing/fixing
There was a problem hiding this comment.
something like passing a memoryview instead of a pointer to kahan_sum ?
There was a problem hiding this comment.
I think it has to go here (and in Generator) since p has to be converted from other iteable containers anyway, The downside of having two very similar files.
If this also appears in multinomial, then it would be best to create a new function in common.pyx, that would do the conversion and the checks (or at least as many as can be done).
|
@bashtage: any thoughts? |
|
There are 3 required casts in each file. The two probabilities and alpha in Dirichlet. The other OFT are fine without the copy since they are all run through broadcast which takes care of the stride. Here is a PR on randomgen that shows the locations: |
|
thanks for the locations @bashtage |
|
In tests of Generator you have to use |
|
I just saw that, I naively copy-pasted. I'm on it :) |
|
just a question: why didn't that fail in randomgen ? you used the old api for the generator tests. |
|
The APIs are not perfectly in-sync since randomgen has legacy (some users), and there is no need to break their code if they wish to upgrade. NumPy gets to start with a clean sheet. |
|
I see, thanks. |
|
Note to self: backport will need to ignore new random generator. |
|
@bashtage Thanks for the heads up. |
This is probably only about 70% true. There is a lot of derived code that is effectively common with 1.16. |
|
Thanks @jeremiedbb, reviewers. |
Fixes #13713
ping @glemaitre