search

phetsims / plinko-probability

"Plinko Probability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 7 forks source link

Use reproducible random number generators #55

Closed pixelzoom closed 8 years ago

pixelzoom commented 8 years ago

Related to https://github.com/phetsims/phet-io/issues/551... I see that this sim uses new Random and Math.random in a few places. So playback/record will not be reproducible. Recommended to address this before publishing, since it's relatively simple. @ariel-phet your call. @samreid feel free to chime in.

pixelzoom commented 8 years ago

TODO:

• Change 1 instance of Math.random to phet.joist.random.nextDouble (search for "Math.random") • Change 2 instances of Random.random to phet.joist.random.random (search for "var Random")

samreid commented 8 years ago

Yes, this is now part of the code review and would be nice to test if Plinko is going to get another full QA cycle.

ariel-phet commented 8 years ago

Yes, please address, thanks for pointing it out

pixelzoom commented 8 years ago

I replaced Math.random and new Random. I also used phet.joist.random.nextBoolean in a couple of places where expression ( random.random() > 0.5 ) was used.

@samreid Would you mind taking a peek, to see if this looks ready for record/playback?

samreid commented 8 years ago

Looks good, I tested in the playback harness and everything was playing back perfectly. In my preceding commits, I deprecated Random.random and switched plinko to use Random.nextDouble instead. If all is well, you may close this issue, @pixelzoom

pixelzoom commented 8 years ago

If Random.random is deprecated, then perhaps it shouldn't be used to implement 3 of Random's other functions. I.e., swap implementations of random and nextDouble, then use nextDouble internally. @samreid what do you think?

samreid commented 8 years ago

Good plan, swapped above.

pixelzoom commented 8 years ago

... and switched to using nextDouble internally in this commit: https://github.com/phetsims/dot/commit/296008e9a0c69a790321db8f96bc7ec78e0e0962

Looks good. Closing.