commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Emmanuel Bourg <ebo...@apache.org>
Subject Re: [VOTE][RC1] Release Commons Rng 1.0
Date Wed, 14 Sep 2016 15:04:53 GMT
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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 



Mime
View raw message