At least one of the distributions could be fixed without recursion at the cost of substantially changing the stream. I thought this is a bad idea since the recursion will only be called 1 in 10**53 draws.

xref #13361

If this goes in, it should have a release note. Given that changing the stream is unlikely, and that rejection algorithms have (unlikely) problems with maintaining reproducible streams due to compiler/platform quirks in any case, I am OK with this fix. I'll let @rkern make the decision.

I think these all only reject if the integer value(s) is exactly (int) 0.

Would the release note be 1.16.4 or 1.17.0, or both? If this gets accepted into mtrand then I'll port it to the RandomState in #13163

In this PR the release note should be 1.17.0. When backported, the person doing the backport should add the 1.16.x note.

After the release of that version, the entire release note will be forward-merged to master.

LGTM. @rkern any thoughts?

After the new PRNG system #13163 gets merged, NEP 19 says that these kinds of changes to the legacy distributions are forbidden. They get frozen, bugs and all. What to do up until the minute #13163 gets merged is formally up in the air, but it seems near enough to me to lean towards not making these changes.

My mind might be changed if there were a report of someone actually running into this and having a problem because of it, and the next release would have this change but not #13163, but I think we only have theoretical issues.

The main reason why I suggested fixing this now, which is the last chance, is that it simplifies the shared code between the future RandomState and RandomGenerator. If this isn't fixed, then the fixed version of these functions will need be available for RandomGenerator while the broken version will need to be retailed for RandomState.

I suppose one could argue that a fix that hardly ever matters is a change that is hardly ever there. @rkern What do you think of the code argument?

Thanks @bashtage .

The distributions code is not shared between RandomState and RandomGenerator. Fixing the distributions for RandomState does not help RandomGenerator. There is no pressure to fix RandomState for the sake of RandomGenerator.

Or at least, I thought we had agreed to do so: #13163 (comment)

My argument was meant to apply with respect to the C distributions code too. Completely sever the two.

I guess with just the C distributions, my argument doesn't matter as much since we can just add new C functions to the same file and point RandomGenerator to the new functions as we evolve them. That's less problematic than the sharing of methods via subclassing.

Nonetheless, in that scheme, it was still unnecessary to make this change to RandomState. It would be fine to have had random_beta() for the legacy RandomState and add the new random_beta_2() for the RandomGenerator. We'll be doing that more and more going forward. It's not costly. RandomState should never hold us back from evolving RandomGenerator.

The distributions code is not shared between RandomState and RandomGenerator. Fixing the distributions for RandomState does not help RandomGenerator. There is no pressure to fix RandomState for the sake of RandomGenerator.

@rkern This isn't how it has been implemented (as of now). They share when the function is common and split when a break occurs. This simplifies having 2 copies around of these 5 distributions (for now).