feat(random/unstable): basic randomization functions #5626

lionel-rowe · 2024-08-03T15:17:34Z

Currently includes the following:

@std/random/between: randomBetween function
@std/random/integer-between: randomIntegerBetween function
@std/random/sample: enhanced version of (to-be-deprecated?) @std/collections/sample's sample function, with new weights and random options
@std/random/seeded: randomSeeded function implementing the PGC32 algorithm
@std/random/shuffle: shuffle function implementing Fisher–Yates shuffle

codecov · 2024-08-03T15:20:01Z

Codecov Report

Attention: Patch coverage is 98.24561% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.24%. Comparing base (7c0e917) to head (6bb99e4).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
random/sample.ts	91.66%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #5626    +/-   ##
========================================
  Coverage   96.24%   96.24%            
========================================
  Files         485      491     +6     
  Lines       39269    39440   +171     
  Branches     5787     5811    +24     
========================================
+ Hits        37793    37958   +165     
- Misses       1432     1438     +6     
  Partials       44       44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

timreichen · 2024-08-03T18:53:24Z

@std/random/pick: pick and pickWeighted functions

Note that we already have sample() in @std/collections. So if we introduced a weighted sample() function, it should be added to @std/collections or even as an option to the current sample() function:

const numbers = [1, 2, 3, 4];
const random = sample(numbers, { weights: [1, 9999, 2, 8888] });

@std/random/seeded: SeededPrng class implementing the Wichman-Hill algorithm

Is there a particular reason why this algorithm is added?

@std/random/shuffle: shuffle function implementing Fisher–Yates shuffle

I think this would be a cool addition to @std/collections.

lionel-rowe · 2024-08-04T02:49:10Z

@timreichen

@std/random/seeded: SeededPrng class implementing the Wichman-Hill algorithm

Is there a particular reason why this algorithm is added?

Only that it was the algorithm suggested in the linked issue, it has a period high enough for any sensible use case, and it seems to give a good distribution of results. One other plus is that arbitrarily chosen positive integer seeds still seem to give good entropy, even if they're apparently "low entropy" numbers like 1 or 12 (i.e. they don't need to be primes or anything fancy). [Edit: Seeds that are adjacent to each other do seem to give very similar first-few results though.] I don't have the requisite expertise to verify its randomness properties beyond "seems to give good results" though.

Edit 2: Upon looking into it further I think something like PCG64 (numpy's default) would be a better choice.
Edit 3: PCG32, as the "recommended for most users" version of PCG.

lionel-rowe · 2024-08-04T05:14:47Z

@std/random/pick: pick and pickWeighted functions

Note that we already have sample() in @std/collections. So if we introduced a weighted sample() function, it should be added to @std/collections or even as an option to the current sample() function:
const numbers = [1, 2, 3, 4];
const random = sample(numbers, { weights: [1, 9999, 2, 8888] });

I quite like the idea of making it an option. It does require "unzipping" the values from their weights, but it can still be pretty versatile in terms of usage:

const weighted = new Map([["a", 5], ["b", 3], ["c", 2]]);
const result = sample([...weighted.keys()], { weights: [...weighted.values()] });

I'll keep the collections stuff within the scope of this PR for now as some of it relies on the random stuff (e.g. I'll use SeededPrng for tests), but happy to split it out before merging if that's preferable.

collections/shuffle.ts

… random

kt3k · 2024-08-07T09:28:11Z

@lionel-rowe Can you move shuffle and this version of sample to std/random (leaving the existing collections/sample untouched)?

Also I'd prefer to see SeededPrng exported from @std/random/seeded-prng, randomBetween from @std/random/between, randomIntegerBetween from @std/random/integer-between for consistency of the rest of std.

collections/sample.ts

kt3k · 2024-08-07T09:38:35Z

random/between_test.ts

+Deno.test("randomBetween() throws if min or max are NaN", () => {
+  assertThrows(() => randomBetween(NaN, 1), AssertionError);
+  assertThrows(() => randomBetween(1, NaN), AssertionError);
+});
+
+Deno.test("randomBetween() throws if max is less than min", () => {
+  assertThrows(() => randomBetween(10, 1), AssertionError);
+});


Nice checks 👍

timreichen · 2024-08-07T10:25:15Z

@lionel-rowe Can you move shuffle and this version of sample to std/random (leaving the existing collections/sample untouched)?

@kt3k Wouldn't that be confusing to have two sample() functions? Or do you mean to deprecate collections/sample in another PR?

Also I'd prefer to see SeededPrng exported from @std/random/seeded-prng, randomBetween from @std/random/between, randomIntegerBetween from @std/random/integer-between for consistency of the rest of std.

Prng looks weird to me. Maybe it should it be written out as SeededPseudoRandomNumberGenerator?

kt3k · 2024-08-07T11:59:45Z

@kt3k Wouldn't that be confusing to have two sample() functions? Or do you mean to deprecate collections/sample in another PR?

I think we can deprecate collections/sample when we stabilize random/sample.

Also I'd prefer to see SeededPrng exported from @std/random/seeded-prng, randomBetween from @std/random/between, randomIntegerBetween from @std/random/integer-between for consistency of the rest of std.

Prng looks weird to me. Maybe it should it be written out as SeededPseudoRandomNumberGenerator?

Or how about the name like SeededRandom? There is a similarly named npm package https://www.npmjs.com/package/seedrandom

timreichen · 2024-08-07T12:38:48Z

random/_types.ts

+   * randomization.
+   * @default {Math.random}
+   */
+  random: () => number;


I think this option is not very web api-ish. I think it would be better to remove it and add some kind of algorithm option if needed, that calls different functions internally. Doing customRandomFunction() * (max - min) + min seems very trivial.

I think this option is not very web api-ish. I think it would be better to remove it and add some kind of algorithm option if needed, that calls different functions internally. Doing customRandomFunction() * (max - min) + min seems very trivial.

Callbacks seem pretty web API-ish to me, and they're more versatile than specifying a name of an algorithm — with a callback, it's easy to use with third-party randomization functions/sources. But perhaps more importantly, specifying an algorithm name would surely make any and all of the all implemented algorithms into non-tree-shakeable dependencies of any function taking such options.

@kt3k do we have a guide on how to name option functions? I thought thy have a suffix of Fn -> randomFn. Not sure though.

random/seeded.ts

timreichen · 2024-08-07T12:44:25Z

random/seeded.ts

+
+    // For convenience, allowing destructuring and direct usage in callbacks
+    // equivalently to Math.random()
+    this.random = this.random.bind(this);


I think destructuring can be done without this.

I think destructuring can be done without this.

Destructuring loses this context of unbound methods (as does passing by reference in a callback).

timreichen · 2024-08-07T12:46:16Z

random/seeded.ts

+   * assertEquals(prng.state, [13062938915293834817n, 10846994826184652623n]);
+   * ```
+   */
+  get state(): Readonly<State> {


Does state need to be accessible outside the class? I only see it for testing purposes.

I think state should definitely be gettable, as it allows persisting a PRNG's current state for resumption later on. It's arguable whether or not it needs to be settable though, perhaps setting it should only be possible via the constructor or a from static method.

As with seeding, I'm still not too settled on how this should be exposed, maybe it should have explicit state and inc properties (and probably an algorithm property too, even though there's only one currently implemented).

Edit: I've kept it settable for now (in case it's desirable to have a global PRNG that can be mutated) and also added fromState static method to get a new instance from state. The SeededRandomState format is now { algorithm, state, inc }. I don't really love that it's state.state, open to suggestions on better naming (notable that the numpy+randomgen implementation is even worse in this respect as it has state.state.state 🫣)

Also notable is that the state and inc properties of SeededRandomState are both bigints, representing uint64s. It's a little unfortunate that this means they can't directly be JSON.stringifyed, as JSON.stringify rejects bigints. However, they can still be serialized/deserialized in some other way or stored in Deno KV, indexedDB, etc.

iuioiua

Let's try get this landed soon.

random/integer_between.ts

random/_types.ts

iuioiua · 2024-09-02T23:56:42Z

random/sample.ts

Sorry, I don't follow.

random/seeded.ts

iuioiua · 2024-09-03T00:03:06Z

random/seeded.ts

This seems a lot nicer. Right now, we hardcode the PCG32 algorithm. Should this instead be configurable? In other words, what if we allowed for BYO PRNG algorithm? As to not delay this PR, we can explore this in a follow-up issue/PR.

Yeah there should be plenty of room to add other options in future. Possibly moving back to a class-based model for the algorithm itself will be desirable (but keep randomSeeded as a HOF). Then roughly, algorithm classes compatible with randomSeeded must implement an interface providing static methods like fromSeed, instance methods like nextUint, and static properties like numSeedBytes and nextUintBitSize.

random/between.ts

iuioiua

One last thing. Can you please go through all exceptions and ensure they following the contributing guidelines? See https://github.com/denoland/std/blob/main/archive/tar_stream.ts for an example.

random/between.ts

lionel-rowe · 2024-09-04T05:05:18Z

One last thing. Can you please go through all exceptions and ensure they following the contributing guidelines? See https://github.com/denoland/std/blob/main/archive/tar_stream.ts for an example.

@iuioiua I think they already do, with the exception that error messages beginning with a variable name don't capitalize the variable name:

throw new RangeError("max must be greater than or equal to min");

How should those be handled? `max`/`min` with backticks would be another option.

random/between.ts

kt3k

Suggested some updates in error messages based on the style guide at https://github.com/denoland/std/blob/1432018f0080422453103994513c782aaf180959/.github/CONTRIBUTING.md#error-messages

random/between.ts

random/integer_between.ts

random/sample.ts

Co-authored-by: Yoshiya Hinosawa <stibium121@gmail.com>

iuioiua

LGTM! Excellent work. Thank you, and thank you to the contributors that chimmed in. Let's track any possible points of improvements in a tracking issue for discussions, etc.

kt3k

LGTM!

feat(random/unstable): basic randomization functions

19b6ed7

lionel-rowe requested a review from kt3k as a code owner August 3, 2024 15:17

Better validation, module docs

f5fa351

lionel-rowe force-pushed the random branch from 8c40211 to f5fa351 Compare August 4, 2024 04:28

Move shuffle and pick/pickWeighted->sample to collections

09b2861

github-actions bot added the collections label Aug 4, 2024

lionel-rowe added 3 commits August 4, 2024 13:34

Validate length of weights array

7d3e287

Validate min and max for between functions

1200129

Update Lint PR title scopes

9208fe1

babiabeo reviewed Aug 4, 2024

View reviewed changes

collections/shuffle.ts Outdated Show resolved Hide resolved

kt3k mentioned this pull request Aug 5, 2024

chore: add random scope in title ci check #5629

Merged

kt3k and others added 5 commits August 5, 2024 11:37

Merge branch 'main' into random

d39753c

Change PRNG algorithm from Wichman-Hill to PCG32

82a0c00

Merge branch 'random' of https://github.com/lionel-rowe/deno_std into…

276cc3c

… random

Xor seeds with FNV-1a for better results with low-entropy seeds

6dc1b3f

Merge branch 'main' into random

8bbd63e

kt3k reviewed Aug 7, 2024

View reviewed changes

collections/sample.ts Outdated Show resolved Hide resolved

kt3k reviewed Aug 7, 2024

View reviewed changes

timreichen reviewed Aug 7, 2024

View reviewed changes

lionel-rowe added 2 commits August 8, 2024 12:00

Change seed + state formats and implement pcg32_srandom_r for seeding

3f55d14

Naming changes + move sample/shuffle back into random

b501ced

github-actions bot removed the collections label Aug 8, 2024

Merge branch 'main' into random

ae7ef2a

lionel-rowe added 3 commits August 31, 2024 00:08

Use built-in BigInt.asUintN for uint wrapping

0c912c7

Copy edits

be0d89a

Simplify + reduce function calls

c2c934a

iuioiua suggested changes Sep 3, 2024

View reviewed changes

lionel-rowe and others added 7 commits September 3, 2024 11:00

Merge branch 'main' into random

e501043

Allow non-integer min+max for randomIntegerBetween

dbb7f94

Add note about algorithm used to randomSeeded

18442d6

Simplify weights example for sample

8980fec

refactor: add @experimental tag to module docs

3a80b0e

refactor: minor cleanups

003d2ac

refactor: minor cleanups

f226f4c

iuioiua suggested changes Sep 3, 2024

View reviewed changes

0f-0b reviewed Sep 3, 2024

View reviewed changes

random/between.ts Outdated Show resolved Hide resolved

Fix randomBetween overflow and rounding issues

9dcb837

lionel-rowe force-pushed the random branch from 9052d98 to 9dcb837 Compare September 4, 2024 04:58

kt3k reviewed Sep 4, 2024

View reviewed changes

random/between.ts Outdated Show resolved Hide resolved

kt3k reviewed Sep 4, 2024

View reviewed changes

random/between.ts Outdated Show resolved Hide resolved

random/between.ts Outdated Show resolved Hide resolved

random/integer_between.ts Outdated Show resolved Hide resolved

random/sample.ts Outdated Show resolved Hide resolved

random/sample.ts Outdated Show resolved Hide resolved

lionel-rowe and others added 5 commits September 4, 2024 17:54

Change sample input types to ArrayLike to allow typed arrays

db2477f

Apply suggestions from code review

82cc66f

Co-authored-by: Yoshiya Hinosawa <stibium121@gmail.com>

Merge branch 'main' into random

d790759

deno fmt

0088886

refactor: improve error handling

6b4b79a

iuioiua approved these changes Sep 5, 2024

View reviewed changes

iuioiua enabled auto-merge (squash) September 5, 2024 00:27

iuioiua disabled auto-merge September 5, 2024 00:27

iuioiua requested a review from kt3k September 5, 2024 00:27

Merge branch 'main' into random

6bb99e4

kt3k approved these changes Sep 5, 2024

View reviewed changes

kt3k merged commit 149839b into denoland:main Sep 5, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(random/unstable): basic randomization functions #5626

feat(random/unstable): basic randomization functions #5626

lionel-rowe commented Aug 3, 2024 •

edited

Loading

codecov bot commented Aug 3, 2024 •

edited

Loading

timreichen commented Aug 3, 2024 •

edited

Loading

lionel-rowe commented Aug 4, 2024 •

edited

Loading

lionel-rowe commented Aug 4, 2024

kt3k commented Aug 7, 2024

kt3k Aug 7, 2024

timreichen commented Aug 7, 2024

kt3k commented Aug 7, 2024

timreichen Aug 7, 2024

lionel-rowe Aug 7, 2024

timreichen Aug 9, 2024

timreichen Aug 7, 2024

lionel-rowe Aug 7, 2024

timreichen Aug 7, 2024

lionel-rowe Aug 7, 2024 •

edited

Loading

iuioiua left a comment

iuioiua Sep 2, 2024

iuioiua Sep 3, 2024

lionel-rowe Sep 3, 2024 •

edited

Loading

iuioiua left a comment

lionel-rowe commented Sep 4, 2024

kt3k left a comment

iuioiua left a comment

kt3k left a comment

feat(random/unstable): basic randomization functions #5626

feat(random/unstable): basic randomization functions #5626

Conversation

lionel-rowe commented Aug 3, 2024 • edited Loading

codecov bot commented Aug 3, 2024 • edited Loading

Codecov Report

timreichen commented Aug 3, 2024 • edited Loading

lionel-rowe commented Aug 4, 2024 • edited Loading

lionel-rowe commented Aug 4, 2024

kt3k commented Aug 7, 2024

Choose a reason for hiding this comment

timreichen commented Aug 7, 2024

kt3k commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lionel-rowe Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

iuioiua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lionel-rowe Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

iuioiua left a comment

Choose a reason for hiding this comment

lionel-rowe commented Sep 4, 2024

kt3k left a comment

Choose a reason for hiding this comment

iuioiua left a comment

Choose a reason for hiding this comment

kt3k left a comment

Choose a reason for hiding this comment

lionel-rowe commented Aug 3, 2024 •

edited

Loading

codecov bot commented Aug 3, 2024 •

edited

Loading

timreichen commented Aug 3, 2024 •

edited

Loading

lionel-rowe commented Aug 4, 2024 •

edited

Loading

lionel-rowe Aug 7, 2024 •

edited

Loading

lionel-rowe Sep 3, 2024 •

edited

Loading