specs icon indicating copy to clipboard operation
specs copied to clipboard

NX endorses spec7 - Seeding Pseudo-Random Number Generation

Open jarrodmillman opened this issue 1 year ago • 1 comments

We are still discussing this SPEC and decided to open this PR to have a place where we can make comments and vote. We will leave this PR open for a week or two.

jarrodmillman avatar Oct 23 '24 16:10 jarrodmillman

I approved this PR for NetworkX endorsement of SPEC7 understanding that it applies only to the API for NumPy random number generators (as stated in the "Scope" part of the SPEC. The first paragraph of the SPEC seems to be much broader in scope than NumPy rngs, but the Scope section does reduce attention to NumPy which is what most of this ecosystem will be using.

I don't think the NetworkX will exactly follow the recommended API because of our use of Python's random.random as well as numpy.random and our tooling to provide a unified RNG interface for both. But I think the project endorses the ideas in this SPEC and will work toward them within our other constraints.

dschult avatar Oct 31 '24 15:10 dschult

We got approvals here, so I think we should go ahead with the merge. Note that netlify is unhappiness is unrelated, so we should just roll with it and fix separately if it's an issue on main, too.

bsipocz avatar Jul 01 '25 17:07 bsipocz

that it applies only to the API for NumPy random number generators

Yeah, I agree it does and the use of Python's rng is fine. I suppose it would be nice if:

  • rng= may be a valid spelling in the future to align naming.
  • rng= behaves identically for NumPy new Generators and other seeding like inputs. I don't think it is problematic if you prefer using random.Random internally. It may be nice to support SeedSequence() for a seed, but that doesn't have to match strictly IMO. One way may be to use SeedSequence but for seeding the Python rng rather than the NumPy one: seed = np.random.SeedSequence(rng).generate_state(8).tobytes(); rng = random.Random(seed). (The 8 would be 8 uint32s, so 256 bit of entropy.)

seberg avatar Jul 03 '25 08:07 seberg

Oh, I'm sorry @rossbar for merging this prematurely.

Given the state of things, I feel the recommended API changes in SPEC 7 don't necessarily map cleanly onto NetworkX specifically.

Yes, I saw that in Dan's comment above, and that nevertheless there is a will to endorse the SPEC. I believe we are saying it from the beginning that endorsing doesn't mean to exactly do what the spec says.

bsipocz avatar Jul 03 '25 15:07 bsipocz

Oh, I'm sorry @rossbar for merging this prematurely.

No worries at all, the merge definitely wasn't premature on your end, the comment was extremely delayed on my end :upside_down_face:

Yes, I saw that in Dan's comment above, and that nevertheless there is a will to endorse the SPEC. I believe we are saying it from the beginning that endorsing doesn't mean to exactly do what the spec says.

Yes this is my understanding of the SPEC process as well - communities can endorse without necessarily adopting, or perhaps modifying adoption as best fits the need of the individual package. I'm very much +1 on endorsing SPEC 7!

rossbar avatar Jul 03 '25 15:07 rossbar