BitGenerator support

[flying-sheep]

See

• https://numpy.org/doc/stable/reference/random/c-api.html
• https://numpy.org/doc/stable/reference/random/extending.html

Fixes #498

The idea is to have a safe wrapper around the npy_bitgen struct that implements rand::RngCore. That way pyo3 functions could be passed a np.random.Generator, get that wrapper from it, and pass it to Rust APIs, which could then call its methods repeatedly.

The way it’s implemented, the workflow would look like this:

1. acquire GIL
2. cast a np.random.BitGenerator instance into a numpy::random::PyBitGenerator.
3. call .lock() on it to get a numpy::random::PyBitGeneratorGuard.
4. release GIL
5. call functions on guard object without needing to hold the GIL

TODO:

• I see local crashes when running all tests, so there’s probably some UB, I’d appreciate help to fix it.

Safety

If somebody releases the threading lock of the BitGenerator while we’re using it, this isn’t safe 🤔

API design options

I could make this more complex by adding a new trait that is implemented by both PyBitGenerator and PyBitGeneratorGuard, allowing to choose if someone wants to

• use the PyBitGenerator’s random_* methods directly on that object while holding the GIL and without locking it
• use it like it’s used now, by locking the np.random.BitGenerator and returning a GIL-free object that can be used.

but for now I just implemented the use case that’s actually desired.

[Icxolu]

This looks like a useful addition! Thanks for working on it. I'm definitely not an expert here, but I left a few comment about things that stood out to me. Let me know what you think.
Also, are there any differences between numpy v1 and v2 that we need to consider?

[flying-sheep]

OK, all addressed!

I removed the 3.9 gating as asked, so CI fails now, maybe I should put it back until the CI for 3.8 is removed?

To have BitGenerator::new be more complete, we need to think about wrapping SeedSequence as well, but we could also leave that for a future PR and just add new with_seed factory methods then, keeping new minimal.

[Icxolu]

Thanks, I'll try to hopefully give this a final review in the coming week.

I removed the 3.9 gating as asked, so CI fails now, maybe I should put it back until the CI for 3.8 is removed?

I've just landed #567 which removes 3.8 support, so a rebase/merge with main should fix CI.

To have BitGenerator::new be more complete, we need to think about wrapping SeedSequence as well, but we could also leave that for a future PR and just add new with_seed factory methods then, keeping new minimal.

I think we should leave seeding (and other additional features) to a followup. This is already a quite a big PR and we should try to land that first and continue from there.

[Icxolu]

I think the implementation is good now. Just a few small suggestions, then this is good to go 🚀

[Icxolu]

Thanks you very much for sticking with me here 🙏 . It took a while but we got there eventually.

[Icxolu]

@mejrs As you have previously looked at this. Do you also want to take another look or are you fine with me merging this?

[mejrs]

That was a long time ago 😊

At a glance it looks OK to me, I'm not going to review it further. I'm fine with it merging.

See #499 (comment)

[flying-sheep]

Nice! I should have gone with your first intuition, which ended up being the end result anyway.

Could have saved us some time, but at least I learned a lot. I was feeling bad for nudging you to save my bad design lol

[Icxolu]

Could have saved us some time, but at least I learned a lot. I was feeling bad for nudging you to save my bad design lol

No worries. Sometimes you need to implement something too see how things play out and occasionally you need to scrap things again, that's just how it goes sometimes. I also learned a thing or two about numpy rngs, and since you also learned something I'd say it's time well spend.