phetsims / states-of-matter

"States of Matter" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
7 stars 8 forks source link

licensing issue with Random.js #100

Closed pixelzoom closed 8 years ago

pixelzoom commented 9 years ago

In Random.js:

/**
 * Replacement for Java's Random.nextGaussian()
 * Code taken from http://developer.classpath.org/doc/java/util/Random-source.html
 *
 * @author Aaron Davis
 */

According to the URL, java.util.Random is in turn taken from GNU Classpath, which is GPL v2.

Various things to address:

  1. Random.js looks general. Should it be moved to common code? Or is there already something similar in common code that could replace it?
  2. An entry for java.util.Random is needed in sherpa/third-party-licenses.json.
  3. The entry created in (2) needs to be added to "licenseKeys" in states-of-matter/package.json, and states-of-matter-basics/package.json if that sim also uses this code.
aaronsamuel137 commented 9 years ago

Random.js is a very incomplete port of Java's Random. It has only one method: nextGaussian. It was basically just enough to do a more direct port of states of matter. Given this, I'm not sure if we should move it to common code or not. But I guess it would make more sense to have it in dot instead of states-of-matter. Do you think I should move it there @pixelzoom? I am not aware of anything in common code that can replace it.

pixelzoom commented 9 years ago

I haven't looked at Random.js, and I don't know how you're using it in SOM, or whether it should move to dot or somewhere else. Perhaps ask @jonathanolson.

There have been several mentions of the need for a random-number generator that can be seeded, for example to support reproducibility of playback. Is this at all related?...

pixelzoom commented 9 years ago

It was basically just enough to do a more direct port of states of matter.

...or if @jbphet is familiar with how Random was used in the Java version of SOM, perhaps he can chime in.

jonathanolson commented 9 years ago

Yes, it would be ideal to have a reseedable PRNG, so that it could be reused during playback (and give the same values).

From scratch, it would be great to have at least something like a Mersenne Twister implementation, with conversion from bits => uniform, then uniform => other transforms (gaussian, etc.). I think @samreid had used something for this previously? I'd also note, it would probably save time to use 3rd party code for whatever parts of that are possible (hopefully everything).

jbphet commented 9 years ago

Here is an MIT-licensed pseudo-random number generator: https://github.com/ckknight/random-js. The readme file is quite thorough and clear, which bodes well for the library itself. Also, in the readme file the author indicates that Math.random could potentially behave differently on different browsers, so incorporating something like this may be essential when we are recording and reusing seeds.

samreid commented 9 years ago

I have previously used sherpa/lib/seedrandom-2.2.js which provides seedable random numbers.

pixelzoom commented 9 years ago

@aaronsamuel137 If SOM can use seedrandom-2.2.js, that might be the path of least resistance. It already has an entry in third-party-licenses.json, and you'd need to add "../sherpa/lib/seedrandom-2.2.js", to "preload" in states-of-matter/package.json and (presumably) states-of-matter-basics/package.json.

aaronsamuel137 commented 9 years ago

It doesn't look like seedrandom has any equivalent of nextGaussian, so I don't see how it can be used here.

I'm seeing that gene-expression-basics also has a partial port of Java's Random - so perhaps these could be moved to consolidated and moved to common code. We'd still need to add the licensing I expect.

I'm not sure what the best long term solution is though. This seems like a different issue than the seedable PRNG issue, since it looks like that can already be accomplished using seedrandom. We could add that functionality to Random.js if we want to continue augmenting that file and moving it to common code.

Perhaps we can discuss at developer meeting?

samreid commented 9 years ago

Here's an implementation of nextGaussian on wikipedia: https://en.wikipedia.org/wiki/Box%E2%80%93Muller_transform

pixelzoom commented 9 years ago

6/18/15 dev meeting:

• Can't use Java's Random.nextGaussian because it's GPL. • Rewrite Random.nextGaussian, see above reference. • Combine SOM and GEB Random.js and move to common-code

aaronsamuel137 commented 9 years ago

Random.js has been moved to common code and now uses Box-Muller for nextGaussian instead of the Java port.

The issues with random seeds have NOT been addressed. This should be a separate issue when the times comes.

The new nextGaussian method seems to be reasonably accurate. Here is a histogram of 2000 samples:

screen shot 2015-06-19 at 9 54 02 am

Assigning to @pixelzoom for review. @jonathanolson feel free to take a look too since you seem to have written a lot of the dot code.

jonathanolson commented 9 years ago

I'd consider nextGaussian for being in Random, since it is used exclusively with random numbers.

Also, it doesn't look like a singleton, but something where you need "new Random()?"

For being in Dot, it should also assign itself to the namespace (see Range as an example), where it imports:

var dot = require( 'DOT/dot' );

setting dot.Range, so it is available in the namespace. A line should also be included in main.js so Random.js is included in Dot-specific builds.

pixelzoom commented 9 years ago

@jonathanolson If it's OK with you, I'm going to assign this to you, to work out the best way to integrate this into dot.

jonathanolson commented 9 years ago

I was curious what the reasoning was for requiring a new (unique) instance, instead of being a collection of static functions, assigning to @aaronsamuel137.

aaronsamuel137 commented 9 years ago

The only reasoning is that is facilitates direct ports from Java, since that keeps the same API as Java's Random. I am open to doing it a different way, but perhaps that should be tracked in a new issue. The main issue here seemed to be getting away from a GPL license, and not necessarily deciding how we want to handle Random in our js code.

aaronsamuel137 commented 9 years ago

I've added Random to the Dot namespace by following the example in Range.

Regarding these comments:

I'd consider nextGaussian for being in Random, since it is used exclusively with random numbers. Also, it doesn't look like a singleton, but something where you need "new Random()?"

I am not totally sure what you mean. nextGaussian is part of Random - but it uses boxMullerTransform(), which I have put in Util at @samreid's suggestion. Are you suggesting this go into Random as well?

Assigning to @jonathanolson

jonathanolson commented 9 years ago

The only reasoning is that is facilitates direct ports from Java, since that keeps the same API as Java's Random.

I was expecting that we'd only need one Instance ever for simulations, since we only want to be using one randomness source. I guess it could be conceivable that a sim needs to control its own seedable randomness source that can be repeated, so it could make sense to have multiple instances.

I am not totally sure what you mean. nextGaussian is part of Random - but it uses boxMullerTransform(), which I have put in Util at @samreid's suggestion. Are you suggesting this go into Random as well?

If Random is not just a collection of functions, but a type with multiple instances, it doesn't make sense to have boxMullerTransform() there.

jbphet commented 8 years ago

The nextGaussian function has been integrated into dot and this sim is now using it. Closing.