[GitHub] [commons-rng] aherbert commented on a change in pull request #85: Minor Changes:

[GitBox <gi...@apache.org>]

aherbert commented on a change in pull request #85:
URL: https://github.com/apache/commons-rng/pull/85#discussion_r620065107



##########
File path: commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/ChengBetaSampler.java
##########
@@ -266,6 +266,7 @@ public double sample() {
                 // Compute Y and Z
                 final double y = u1 * u2;
                 final double z = u1 * y;
+                final double v1 = Math.log(u1) - Math.log1p(-u1);

Review comment:
       If you look at where this computation is done it is not always required in each iteration of the loop. Only step 3 and step 5 require it, and not both at the same time. 50% of the time step 2 is executed and you do not require this value. It is more efficient to compute it when required. So this change will slow down the sampler with 2 unnecessary calls to log functions in the majority of cases.

##########
File path: commons-rng-simple/src/main/java/org/apache/commons/rng/simple/internal/ProviderBuilder.java
##########
@@ -306,9 +306,8 @@ protected Object convertSeed(Object seed) {
                 final long increment = SeedUtils.createLongHexPermutation(source);
                 // The initial state should not be low complexity but the Weyl
                 // state can be any number.
-                final long state = increment;
                 final long weylState = source.nextLong();
-                return new long[] {state, weylState, increment};
+                return new long[] {increment, weylState, increment};

Review comment:
       I agree the code is redundant. But it makes it very clear the returned array is composed of 3 distinct values, the initial state, the state for the Weyl sequence and the increment. This is a micro-optimisation in a method used for a constructor for a long lived object. Performance impact will be zero and code clarity is reduced by the change.

##########
File path: commons-rng-core/src/main/java/org/apache/commons/rng/core/source64/SplitMix64.java
##########
@@ -59,7 +59,7 @@ public SplitMix64(Long seed) {
      * @param seed Seed.
      */
     private void setSeedInternal(Long seed) {
-        state = seed.longValue();
+        this.state = seed;

Review comment:
       This explicit use of `longValue` was discussed when the constructor was added to use a primitive long. The code as is makes it explicit that the input is a Long and not a long. Looking at this now the entire setSeedInternal method is redundant. It is preserved to match other implementations. There are 9 classes in the package with the setSeedInternal method that use the constructor input object argument named seed. The method will be inlined by the JVM anyway (it is less than 35 bytes when in java byte code) and the performance impact is negligible.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org