-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement "random" module (WIP) #1571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
} | ||
|
||
#[pymethod] | ||
fn seed(&self, n: Option<usize>, vm: &VirtualMachine) -> PyResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make n
an Option<u64>
to avoid the cast
tests/snippets/stdlib_random.py
Outdated
right = [2, 7, 3, 5, 8, 4, 6, 9, 0, 1] | ||
random.shuffle(left) | ||
|
||
assert all([l == r for l, r in zip(left, right) ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this same as assert left == right
? Same for other tests too.
#[pyclass(name = "Random")] | ||
#[derive(Debug)] | ||
struct PyRandom { | ||
rng: RefCell<SmallRng> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly recommend to use StdRng
as a default instead, it is a much safer choice, because it is not predictable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be possible to have an enum inside the RefCell
with both StdRng
and SmallRng
as variants, and only switch to SmallRng
when seed
is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SmallRng
is not the Mersenne Twister, so it does not make sense to switch to it for compatibility with CPython. Other than that, I agree!
None => SmallRng::from_entropy(), | ||
Some(n) => { | ||
let seed = n as u64; | ||
SmallRng::seed_from_u64(seed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to support larger seeds (via bigints), but this requires more work and could be done later (likely in a backwards incompatible way).
@malkoG I'd be willing to take over this PR, could I have your permission to use the code you've written as a base? |
Sure |
Functions for integers, functions for sequences works. But, Real-valued distributions is still working on progress.