Use rng initialized with seedrandom (alea) #2992
Conversation
🦋 Changeset detectedLatest commit: 75c1db9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| @@ -81,6 +81,9 @@ const interpolate = ( | |||
| yMax: number, | |||
| xIntended: number | |||
| ): number => { | |||
| if (yMin === yMax) { | |||
| return yMin; | |||
| } | |||
There was a problem hiding this comment.
This is important; yMin * minProportion + yMax * maxProportion was precise with Math.random() values which ended with 0 bit, but not with other RNGs.
| @@ -59,6 +61,7 @@ function throwFrom( | |||
| * `context.evaluate` can inject additional behaviors, e.g. delay for pseudo-async evaluation. | |||
| */ | |||
| export const evaluate: ReducerFn = (expression, context) => { | |||
| jstat.setRandom(context.rng); | |||
There was a problem hiding this comment.
jstat is global, so this is stateful, but as long as evaluate is not async, it's good enough.
There was a problem hiding this comment.
Makes sense. A bit scary, we'll just have to remember if we go async or parallelize
| @@ -40,15 +40,6 @@ export abstract class SqAbstractDistribution<T extends BaseDist> { | |||
| ); | |||
| } | |||
|
|
|||
| asSampleSetDist(env: Env) { | |||
There was a problem hiding this comment.
Instead of requiring rng here, I removed this method, it wasn't used anywhere.
|
|
||
| xDist(env: Env): Result.result<SqSampleSetDistribution, SqError> { | ||
| return this.buildSampleSetDist(this._value.xDist, env); | ||
| xDist(): SqSampleSetDistribution { |
There was a problem hiding this comment.
It's a nice coincidence that scatter plots require sample set dists now, otherwise I'd have to require rng prop, which is hard to obtain in components.
|
Seems pretty great, happy that this was so straightforward. I don't like that this seems to make a regression to Squiggle hub -> Now, there doesn't seem to be any randomness between runs. I could see why we'd want this, but it's not apparent to the user, which is kind of scary to me. I'd expect that every time I click "run", the seed would change, unless I made it fixed. How about now we randomly set the seed each "run", then later we make the seed configurable? Happy for it to be merged otherwise. |
This was actually a bug! Autoseeding didn't work, I fixed it in 75c1db9. |
Fixes #248.
Seed is optional in
environment, and not exposed to the UI yet. Tests are still random too.seedrandom.aleais faster thanMath.random()in Node v20 (I'm not sure about Firefox):