Hi all, RNG is moving fast, that's nice. I got a quick look at the API and the site, here are my comments:
- The main page (Overview) could be enhanced with a description of the API longer than a single sentence. It could mention the motivations for the API, the implementations supported, a sample code snippet and a link to the user guide. - In the Performance section of the User Guide, I'd suggest grouping the 3 tables into a single one. It might also be interesting to put the quality results in the same table, so we can see the trade-off between performance and quality. - The License section of the user guide could be removed, I guess everyone knows that Apache projects use the Apache license. - The internal packages should be hidden from the Javadoc, otherwise users may be tempted to directly use the classes there. This would imply copying the documentation of the generators into the RandomSource class. - Why isn't the UniformRandomProvider interface simply named RandomProvider? Is there any plan to implement non uniform random generators in the future? Will they have a different interface? - UniformRandomProvider mimics the java.util.Random methods which is nice. Is there any value in also implementing the nextGaussian() method such that our API is a perfect drop-in replacement for the JDK class? - For code relying on the java.util.Random type, it might be useful to provide an adapter turning an UniformRandomProvider into a java.util.Random implementation (Hipparchus has one). - The choice of an enum for the factory class RandomSource is a bit unusual. It is probably ok for simple RNG with a simple seed, but with more complex implementations I find it less intuitive to use. For example the create(RandomSource, Object Object...) method has this snippet in its documentation: RandomSource.create(RandomSource.TWO_CMRES_SELECT, 26219, 6, 9) When a user types this in its IDE, there is no type information nor contextual documentation for the parameters expected. The user has to know the type of the seed and the meaning of the parameters expected, the IDE won't be able to help here (with CTRL+P and CTRL+Q in IntelliJ). This design induces some complexity, the loss of the seed type leads to the seed conversion mechanism (most of the util package if I understand well). I'd feel more comfortable with a more classic factory design, something like: RandomSource.createTwoCmres() RandomSource.createTwoCmres(Integer seed) RandomSource.createTwoCmres(Integer seed, int i, int j) or a shorter form like: RNG.newTwoCmres() RNG.newTwoCmres(int seed) RNG.newTwoCmres(Integer seed, int i, int j) That will add more methods to the factory, but it'll be much more usable in my opinion. - I'm not convinced by the need for the convenience static methods RandomSource.createInt/Long. If the user doesn't want to specify the seed, he could simply use the seed-less create method. And if the random generator requires extra parameters, he could use a null value for the seed as a way to mean "generate one for me please": RandomSource.create(RandomSource.TWO_CMRES_SELECT, null, 6, 9) - RandomSource.saveState/restoreState allows one to convert a random generator to/from a byte array. This looks a lot like Java serialization. Maybe the implementations supporting this feature could be marked as Serializable, thus it would be possible to call RandomSource.saveState only if the instance is Serializable instead of catching the UnsupportedOperationException thrown when the feature isn't supported. - Why is the byte[] state encapsulated into the RandomSource.State class? Why not using a byte array directly? So far I'm +1 for a beta release aimed at getting more feedback on the API, but I'm -1 for releasing the current code as 1.0. Emmanuel Bourg Le 11/09/2016 à 16:55, Gilles a écrit : > Hi. > > This is a [VOTE] for releasing Apache Commons Rng 1.0 (from RC1). > > Tag name: > RNG_1_0_RC1 (signature can be checked from git using 'git tag -v') > > Tag URL: > > https://git-wip-us.apache.org/repos/asf?p=commons-rng.git;a=commit;h=f8d23082607b9f2c7be7f489eb09627722440ee5 > > > Commit ID the tag points at: > f8d23082607b9f2c7be7f489eb09627722440ee5 > > Site: > http://home.apache.org/~erans/commons-rng-1.0-RC1-site > > Distribution files: > https://dist.apache.org/repos/dist/dev/commons/rng/ > > Distribution files hashes (SHA1): > a221e862c8ff970a9ca3e7fbd86c3200d1f8780a commons-rng-1.0-bin.tar.gz > 689b2bfbdb1856d4f47851d75762aab42057805a commons-rng-1.0-bin.zip > 40b7b1639eedf91b5fad5d38e6ebec01e659048f commons-rng-1.0-src.tar.gz > 6296dbabde10169d6365bda99f2af6dcc191e515 commons-rng-1.0-src.zip > > KEYS file to check signatures: > http://www.apache.org/dist/commons/KEYS > > Maven artifacts: > > https://repository.apache.org/content/repositories/orgapachecommons-1199/org/apache/commons/commons-rng/1.0/ > > > [ ] +1 Release it. > [ ] +0 Go ahead; I don't care. > [ ] -0 There are a few minor glitches: ... > [ ] -1 No, do not release it because ... > > This vote will close in 72 hours, at 2016-09-14T15:10:00Z (this is UTC > time). > ---------- > > Thanks, > Gilles > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org >
signature.asc
Description: OpenPGP digital signature
