BUG: random: Create a legacy implementation of random.binomial. #14531
Conversation
Create a legacy implementation of the random_binomial method of RandomState that does not include the "short-circuit" check for n == 0 or p == 0. This ensures that the stream of variates is consistent with the behavior in 1.16. Closes numpygh-14522.
|
Grrr. I don't know why the legacy functions were even touched, they should have been taken over exactly as they were and copied if they were needed for the new work so that the two implementations were strictly independent. As is, I see Anyway, just venting. Although It's tempting to undertake a massive redo :) Grrr. Will take some time to work through this. |
|
@charris, yeah, making a fix like this is a bit convoluted, and as I was working on this, I was thinking what you were probably thinking: wouldn't it have been easier to leave the legacy code untouched, and port a copy of it to use int64 for the new Generator class? That would have duplicated a lot of code, but we don't plan on touching the old code much, so the duplication probably wouldn't have been that much of an added maintenance burden. |
I'm thinking we may still want to do that, especially if we want to maintain the old lower level API. But maybe not for 1.17.x, too big a change. There was a small fix to legacy that we did backport, so we will need to keep track of that if we go that way. |
|
Sounds reasonable. I don't have much to add here since I haven't looked under the hood of the new |
|
We ended up here because it went to a. new implementation -> EDIT: Removed wrong info, correction below |
What is wrong with my earlier missive is that the new infrastructure is required if the feature to share a single BitGenerator across both RandomState and Generator. So rolling this back would violate this feature. So keeping randomkit isn't an option. What could be done would be to fully mirror the functions in distributions.c to legacy_distributions.c so that nothing is shared. This might simplify some maintenance later. It would also allow someone to perform the gruesome task of trying to compare a diff between randomkit and a complete legacy_distributions for inadvertant changes. |
|
@mattip Do you know what the coverage reported here: https://codecov.io/gh/numpy/numpy/src/master/numpy/random/src/distributions/distributions.c is so spotty for distributions.c? I'm sure that almost all of that is tested in the standard test suite. For example, random_noncentral_chisquare shows no coverage. However, there is a test here that should hit that path: https://github.com/numpy/numpy/blob/master/numpy/random/tests/test_generator_mt19937.py#L1103 I was thinking using the coverage to see if there are paths that aren't hit so that we could add tests from RandomState that covered this and made sure they passed against 1.16. Edit Looking at the coverage it seems that only tests of RandomState are being executed with coverage, and that tests of Generator are not. This seems to be the case since none of the Ziggurat algos are covered, and these aren't used by RandomState. The other with missing coverage depend on outputs of the Ziggurat and so also aren't covered by tests that hit RandomState. |
|
pinging @tylerjereddy about coverage. It seems something is not right |
|
When I try to get the coverage locally by running I get a warning and no coverage: I have to manually edit the time stamp in the files (offset 8, 4 bytes) so that the gcda's timestamp is after the gcno's. Possibly this is happening because distributions is being compiled multiple times by both the cython build of Generator and RandomState. |
|
Let's put this in and not try to untangle the legacy code at this time. If down the road we run into more, we should discuss reverting the code before digging too much into the exact problem. |
Create a legacy implementation of the random_binomial method of
RandomState that does not include the "short-circuit" check for
n == 0 or p == 0. This ensures that the stream of variates
is consistent with the behavior in 1.16.
Closes gh-14522.