feat(std): add bounded random int to PCG32#1877
Conversation
PabloAndresCQ
left a comment
There was a problem hiding this comment.
Thanks! This looks great. I've double checked against the reference implementation in C. I've also checked locally that the random_bound_int from the reference implementation and the Guppy implementation agree (and that they agree with the values in the test file).
I've made a couple of comments that are not from this PR, but from the original one that @luffy-orf also contributed with.
PS: I'm approving since, from the user perspective, this is sufficient. However, @hsemenenko, please have a look at the interface + my comments on tests, and mark as "Request changes" if you deem it necessary.
hsemenenko
left a comment
There was a problem hiding this comment.
Thanks for adding this, It looks great! I've added a few comments of things to change. None of the comments should require major changes but I think are useful additions.
|
Thanks @hsemenenko all your review comments are addressed in the latest push: _mask64 readability: two_to_32 now uses 1 << 32. For the 64-bit mask, (1 << 64) - 1 triggers a hugr const-fold shift overflow during emulation, so _mask64 combines two 32-bit masks instead (with an inline comment). Ready for another look when you have a moment |
hsemenenko
left a comment
There was a problem hiding this comment.
Looking great, thank you. Apologies for not realising that 1<<64 would overflow, but great job coming up with a work around.
Just one minor change with the bound panic and I can approve.
| :py:meth:`guppylang.std.qsystem.random.RNG.random_int_bounded`. | ||
| """ | ||
| if bound == nat(0): | ||
| panic("PCG32.next_int_bounded: bound must be positive") |
There was a problem hiding this comment.
After a discussion with @PabloAndresCQ, could this be changed to use exit rather than panic? The reason is that exit will only exit the current shot and then continue to the next shot, while panic will exit and not run any subsequent shots.
| panic("PCG32.next_int_bounded: bound must be positive") | |
| exit("PCG32.next_int_bounded: bound must be positive") |
|
@hsemenenko Done - switched to exit for the zero-bound case and updated the test to verify shot 0 exits while shot 1 continues (test_pcg32_bounded_bound_zero_exits). Thanks for catching that distinction with @PabloAndresCQ! |
Summary
PCG32.next_int_bounded(bound)using PCG'spcg32_boundedrand_rrejection sampling (uniform[0, bound)without modulo bias)_next_uint32()shared bynext_int()and bounded drawsFixes [Feature]: Add bounded random int to
std.random#1874Test plan
uv run pytest tests/integration/std/test_random.pyuv run pre-commit run --all-filesuv run pytest -n auto(1614 passed)