Fixed generation of long random numbers between two bounds.

We now directly use discrete raw values to build the int/double instead
of relying on floating point arithmetic.

JIRA: MATH-936

git-svn-id: https://svn.apache.org/repos/asf/commons/proper/math/trunk@1454897 13f79535-47bb-0310-9956-ffa450edef68

src/changes/changes.xml

@@ -55,10 +55,13 @@ This is a minor release: It combines bug fixes and new features.
   Changes to existing features were made in a backwards-compatible
   way such as to allow drop-in replacement of the v3.1[.1] JAR file.
 ">
+      <action dev="luc" type="fix" issue="MATH-936" >
+        Fixed generation of long random numbers between two bounds.
+      </action>
       <action dev="luc" type="fix" issue="MATH-942" due-to="Piotr Wydrych" >
         Fixed creation of generic array.
       </action>
-        <action dev="luc" type="add" issue="MATH-914" >
+      <action dev="luc" type="add" issue="MATH-914" >
         Check bounds in multi-start vector optimizers.
       </action>
       <action dev="luc" type="add" issue="MATH-941" due-to="Piotr Wydrych" >

src/main/java/org/apache/commons/math3/random/BitsStreamGenerator.java

@@ -162,6 +162,31 @@ public abstract class BitsStreamGenerator
         return high | low;
     }
 
+    /**
+     * Returns a pseudorandom, uniformly distributed <tt>long</tt> value
+     * between 0 (inclusive) and the specified value (exclusive), drawn from
+     * this random number generator's sequence.
+     *
+     * @param n the bound on the random number to be returned.  Must be
+     * positive.
+     * @return  a pseudorandom, uniformly distributed <tt>long</tt>
+     * value between 0 (inclusive) and n (exclusive).
+     * @throws IllegalArgumentException  if n is not positive.
+     */
+    public long nextLong(long n) throws IllegalArgumentException {
+        if (n > 0) {
+            long bits;
+            long val;
+            do {
+                bits = ((long) next(31)) << 32;
+                bits = bits | (((long) next(32)) & 0xffffffffL);
+                val  = bits % n;
+            } while (bits - val + (n - 1) < 0);
+            return val;
+        }
+        throw new NotStrictlyPositiveException(n);
+    }
+
     /**
      * Clears the cache used by the default implementation of
      * {@link #nextGaussian}.

src/main/java/org/apache/commons/math3/random/RandomDataGenerator.java

@@ -45,7 +45,6 @@ import org.apache.commons.math3.exception.NotStrictlyPositiveException;
 import org.apache.commons.math3.exception.NumberIsTooLargeException;
 import org.apache.commons.math3.exception.OutOfRangeException;
 import org.apache.commons.math3.exception.util.LocalizedFormats;
-import org.apache.commons.math3.util.FastMath;
 
 /**
  * Implements the {@link RandomData} interface using a {@link RandomGenerator}
@@ -194,25 +193,82 @@ public class RandomDataGenerator implements RandomData, Serializable {
     }
 
     /** {@inheritDoc} */
-    public int nextInt(int lower, int upper) throws NumberIsTooLargeException {
+    public int nextInt(final int lower, final int upper) throws NumberIsTooLargeException {
         if (lower >= upper) {
             throw new NumberIsTooLargeException(LocalizedFormats.LOWER_BOUND_NOT_BELOW_UPPER_BOUND,
                                                 lower, upper, false);
         }
-        double r = getRan().nextDouble();
+        final int max = (upper - lower) + 1;
-        double scaled = r * upper + (1.0 - r) * lower + r;
+        if (max <= 0) {
-        return (int) FastMath.floor(scaled);
+            // the range is too wide to fit in a positive int (larger than 2^31); as it covers
+            // more than half the integer range, we use directly a simple rejection method
+            final RandomGenerator rng = getRan();
+            while (true) {
+                final int r = rng.nextInt();
+                if (r >= lower && r <= upper) {
+                    return r;
+                }
+            }
+        } else {
+            // we can shift the range and generate directly a positive int
+            return lower + getRan().nextInt(max);
+        }
     }
 
     /** {@inheritDoc} */
-    public long nextLong(long lower, long upper) throws NumberIsTooLargeException {
+    public long nextLong(final long lower, final long upper) throws NumberIsTooLargeException {
         if (lower >= upper) {
             throw new NumberIsTooLargeException(LocalizedFormats.LOWER_BOUND_NOT_BELOW_UPPER_BOUND,
                                                 lower, upper, false);
         }
-        double r = getRan().nextDouble();
+        final long max = (upper - lower) + 1;
-        double scaled = r * upper + (1.0 - r) * lower + r;
+        if (max <= 0) {
-        return (long)FastMath.floor(scaled);
+            // the range is too wide to fit in a positive long (larger than 2^63); as it covers
+            // more than half the long range, we use directly a simple rejection method
+            final RandomGenerator rng = getRan();
+            while (true) {
+                final long r = rng.nextLong();
+                if (r >= lower && r <= upper) {
+                    return r;
+                }
+            }
+        } else if (max < Integer.MAX_VALUE){
+            // we can shift the range and generate directly a positive int
+            return lower + getRan().nextInt((int) max);
+        } else {
+            // we can shift the range and generate directly a positive long
+            return lower + nextLong(getRan(), max);
+        }
+    }
+
+    /**
+     * Returns a pseudorandom, uniformly distributed <tt>long</tt> value
+     * between 0 (inclusive) and the specified value (exclusive), drawn from
+     * this random number generator's sequence.
+     *
+     * @param n the bound on the random number to be returned.  Must be
+     * positive.
+     * @return  a pseudorandom, uniformly distributed <tt>long</tt>
+     * value between 0 (inclusive) and n (exclusive).
+     * @throws IllegalArgumentException  if n is not positive.
+     */
+    private static long nextLong(final RandomGenerator rng, final long n) throws IllegalArgumentException {
+        if (n > 0) {
+            final byte[] byteArray = new byte[8];
+            long bits;
+            long val;
+            do {
+                rng.nextBytes(byteArray);
+                bits = 0;
+                for (final byte b : byteArray) {
+                    bits = (bits << 8) | (((long) b) & 0xffL);
+                }
+                bits = bits & 0x7fffffffffffffffL;
+                val  = bits % n;
+            } while (bits - val + (n - 1) < 0);
+            return val;
+        }
+        throw new NotStrictlyPositiveException(n);
     }
 
     /**
@@ -282,27 +338,82 @@ public class RandomDataGenerator implements RandomData, Serializable {
     }
 
     /**  {@inheritDoc} */
-    public int nextSecureInt(int lower, int upper) throws NumberIsTooLargeException {
+    public int nextSecureInt(final int lower, final int upper) throws NumberIsTooLargeException {
         if (lower >= upper) {
             throw new NumberIsTooLargeException(LocalizedFormats.LOWER_BOUND_NOT_BELOW_UPPER_BOUND,
                                                 lower, upper, false);
         }
-        SecureRandom sec = getSecRan();
+        final int max = (upper - lower) + 1;
-        final double r = sec.nextDouble();
+        if (max <= 0) {
-        final double scaled = r * upper + (1.0 - r) * lower + r;
+            // the range is too wide to fit in a positive int (larger than 2^31); as it covers
-        return (int)FastMath.floor(scaled);
+            // more than half the integer range, we use directly a simple rejection method
+            final SecureRandom rng = getSecRan();
+            while (true) {
+                final int r = rng.nextInt();
+                if (r >= lower && r <= upper) {
+                    return r;
+                }
+            }
+        } else {
+            // we can shift the range and generate directly a positive int
+            return lower + getSecRan().nextInt(max);
+        }
     }
 
     /** {@inheritDoc} */
-    public long nextSecureLong(long lower, long upper) throws NumberIsTooLargeException {
+    public long nextSecureLong(final long lower, final long upper) throws NumberIsTooLargeException {
         if (lower >= upper) {
             throw new NumberIsTooLargeException(LocalizedFormats.LOWER_BOUND_NOT_BELOW_UPPER_BOUND,
                                                 lower, upper, false);
         }
-        SecureRandom sec = getSecRan();
+        final long max = (upper - lower) + 1;
-        final double r = sec.nextDouble();
+        if (max <= 0) {
-        final double scaled = r * upper + (1.0 - r) * lower + r;
+            // the range is too wide to fit in a positive long (larger than 2^63); as it covers
-        return (long)FastMath.floor(scaled);
+            // more than half the long range, we use directly a simple rejection method
+            final SecureRandom rng = getSecRan();
+            while (true) {
+                final long r = rng.nextLong();
+                if (r >= lower && r <= upper) {
+                    return r;
+                }
+            }
+        } else if (max < Integer.MAX_VALUE){
+            // we can shift the range and generate directly a positive int
+            return lower + getSecRan().nextInt((int) max);
+        } else {
+            // we can shift the range and generate directly a positive long
+            return lower + nextLong(getSecRan(), max);
+        }
+    }
+
+    /**
+     * Returns a pseudorandom, uniformly distributed <tt>long</tt> value
+     * between 0 (inclusive) and the specified value (exclusive), drawn from
+     * this random number generator's sequence.
+     *
+     * @param n the bound on the random number to be returned.  Must be
+     * positive.
+     * @return  a pseudorandom, uniformly distributed <tt>long</tt>
+     * value between 0 (inclusive) and n (exclusive).
+     * @throws IllegalArgumentException  if n is not positive.
+     */
+    private static long nextLong(final SecureRandom rng, final long n) throws IllegalArgumentException {
+        if (n > 0) {
+            final byte[] byteArray = new byte[8];
+            long bits;
+            long val;
+            do {
+                rng.nextBytes(byteArray);
+                bits = 0;
+                for (final byte b : byteArray) {
+                    bits = (bits << 8) | (((long) b) & 0xffL);
+                }
+                bits = bits & 0x7fffffffffffffffL;
+                val  = bits % n;
+            } while (bits - val + (n - 1) < 0);
+            return val;
+        }
+        throw new NotStrictlyPositiveException(n);
     }
 
     /**

src/main/java/org/apache/commons/math3/stat/ranking/NaturalRanking.java

@@ -24,8 +24,7 @@ import java.util.List;
 
 import org.apache.commons.math3.exception.MathInternalError;
 import org.apache.commons.math3.exception.NotANumberException;
-import org.apache.commons.math3.random.RandomData;
+import org.apache.commons.math3.random.RandomDataGenerator;
-import org.apache.commons.math3.random.RandomDataImpl;
 import org.apache.commons.math3.random.RandomGenerator;
 import org.apache.commons.math3.util.FastMath;
 
@@ -84,7 +83,7 @@ public class NaturalRanking implements RankingAlgorithm {
     private final TiesStrategy tiesStrategy;
 
     /** Source of random data - used only when ties strategy is RANDOM */
-    private final RandomData randomData;
+    private final RandomDataGenerator randomData;
 
     /**
      * Create a NaturalRanking with default strategies for handling ties and NaNs.
@@ -105,7 +104,7 @@ public class NaturalRanking implements RankingAlgorithm {
         super();
         this.tiesStrategy = tiesStrategy;
         nanStrategy = DEFAULT_NAN_STRATEGY;
-        randomData = new RandomDataImpl();
+        randomData = new RandomDataGenerator();
     }
 
     /**
@@ -130,7 +129,7 @@ public class NaturalRanking implements RankingAlgorithm {
         super();
         this.nanStrategy = nanStrategy;
         this.tiesStrategy = tiesStrategy;
-        randomData = new RandomDataImpl();
+        randomData = new RandomDataGenerator();
     }
 
     /**
@@ -143,7 +142,7 @@ public class NaturalRanking implements RankingAlgorithm {
         super();
         this.tiesStrategy = TiesStrategy.RANDOM;
         nanStrategy = DEFAULT_NAN_STRATEGY;
-        randomData = new RandomDataImpl(randomGenerator);
+        randomData = new RandomDataGenerator(randomGenerator);
     }
 
 
@@ -159,7 +158,7 @@ public class NaturalRanking implements RankingAlgorithm {
         super();
         this.nanStrategy = nanStrategy;
         this.tiesStrategy = TiesStrategy.RANDOM;
-        randomData = new RandomDataImpl(randomGenerator);
+        randomData = new RandomDataGenerator(randomGenerator);
     }
 
     /**

src/test/java/org/apache/commons/math3/random/MersenneTwisterTest.java

@@ -23,7 +23,7 @@ public class MersenneTwisterTest extends RandomGeneratorAbstractTest {
 
     @Override
     protected RandomGenerator makeGenerator() {
-        return new MersenneTwister(100);
+        return new MersenneTwister(111);
     }
     
     // TODO: Some of the tests moved up to RandomGeneratorAbstractTest tested alternative seeding / constructors

src/test/java/org/apache/commons/math3/random/RandomDataGeneratorTest.java

@@ -119,6 +119,7 @@ public class RandomDataGeneratorTest {
         for (int i = 0; i < 5; i++) {
             checkNextIntUniform(-7, -4);
             checkNextIntUniform(-15, -2);
+            checkNextIntUniform(Integer.MIN_VALUE + 1, Integer.MIN_VALUE + 12);
         }
     }
 
@@ -128,10 +129,10 @@ public class RandomDataGeneratorTest {
             checkNextIntUniform(0, 3);
             checkNextIntUniform(2, 12);
             checkNextIntUniform(1,2);
+            checkNextIntUniform(Integer.MAX_VALUE - 12, Integer.MAX_VALUE - 1);
         }
     }
 
-    
     private void checkNextIntUniform(int min, int max) {
         final Frequency freq = new Frequency();
         for (int i = 0; i < smallSampleSize; i++) {
@@ -152,6 +153,24 @@ public class RandomDataGeneratorTest {
         TestUtils.assertChiSquareAccept(expected, observed, 0.001);
     }
 
+    @Test
+    public void testNextIntWideRange() {
+        int lower = -0x6543210F;
+        int upper =  0x456789AB;
+        int max   = Integer.MIN_VALUE;
+        int min   = Integer.MAX_VALUE;
+        for (int i = 0; i < 1000000; ++i) {
+            int r = randomData.nextInt(lower, upper);
+            max = FastMath.max(max, r);
+            min = FastMath.min(min, r);
+            Assert.assertTrue(r >= lower);
+            Assert.assertTrue(r <= upper);
+        }
+        double ratio = (((double) max)   - ((double) min)) /
+                       (((double) upper) - ((double) lower));
+        Assert.assertTrue(ratio > 0.99999);
+    }
+    
     @Test
     public void testNextLongIAE() {
         try {
@@ -175,6 +194,7 @@ public class RandomDataGeneratorTest {
         for (int i = 0; i < 5; i++) {
             checkNextLongUniform(-7, -4);
             checkNextLongUniform(-15, -2);
+            checkNextLongUniform(Long.MIN_VALUE + 1, Long.MIN_VALUE + 12);
         }
     }
 
@@ -183,17 +203,19 @@ public class RandomDataGeneratorTest {
         for (int i = 0; i < 5; i++) {
             checkNextLongUniform(0, 3);
             checkNextLongUniform(2, 12);
+            checkNextLongUniform(Long.MAX_VALUE - 12, Long.MAX_VALUE - 1);
         }
     }
 
-    private void checkNextLongUniform(int min, int max) {
+    private void checkNextLongUniform(long min, long max) {
         final Frequency freq = new Frequency();
         for (int i = 0; i < smallSampleSize; i++) {
             final long value = randomData.nextLong(min, max);
-            Assert.assertTrue("nextLong range", (value >= min) && (value <= max));
+            Assert.assertTrue("nextLong range: " + value + " " + min + " " + max,
+                              (value >= min) && (value <= max));
             freq.addValue(value);
         }
-        final int len = max - min + 1;
+        final int len = ((int) (max - min)) + 1;
         final long[] observed = new long[len];
         for (int i = 0; i < len; i++) {
             observed[i] = freq.getCount(min + i);
@@ -206,6 +228,24 @@ public class RandomDataGeneratorTest {
         TestUtils.assertChiSquareAccept(expected, observed, 0.01);
     }
 
+    @Test
+    public void testNextLongWideRange() {
+        long lower = -0x6543210FEDCBA987L;
+        long upper =  0x456789ABCDEF0123L;
+        long max = Long.MIN_VALUE;
+        long min = Long.MAX_VALUE;
+        for (int i = 0; i < 10000000; ++i) {
+            long r = randomData.nextLong(lower, upper);
+            max = FastMath.max(max, r);
+            min = FastMath.min(min, r);
+            Assert.assertTrue(r >= lower);
+            Assert.assertTrue(r <= upper);
+        }
+        double ratio = (((double) max)   - ((double) min)) /
+                       (((double) upper) - ((double) lower));
+        Assert.assertTrue(ratio > 0.99999);
+    }
+    
     @Test
     public void testNextSecureLongIAE() {
         try {

src/test/java/org/apache/commons/math3/random/Well512aTest.java

@@ -23,7 +23,7 @@ public class Well512aTest extends RandomGeneratorAbstractTest {
     
     @Override
     public RandomGenerator makeGenerator() {
-        return new Well512a(100);
+        return new Well512a(101);
     }
     @Test
     public void testReferenceCode() {

src/test/java/org/apache/commons/math3/stat/ranking/NaturalRankingTest.java

@@ -176,22 +176,22 @@ public class NaturalRankingTest {
         NaturalRanking ranking = new NaturalRanking(NaNStrategy.FIXED,
                 randomGenerator);
         double[] ranks = ranking.rank(exampleData);
-        double[] correctRanks = { 5, 4, 6, 7, 3, 8, Double.NaN, 1, 4 };
+        double[] correctRanks = { 5, 3, 6, 7, 3, 8, Double.NaN, 1, 2 };
         TestUtils.assertEquals(correctRanks, ranks, 0d);
         ranks = ranking.rank(tiesFirst);
-        correctRanks = new double[] { 1, 1, 4, 3, 5 };
+        correctRanks = new double[] { 1, 2, 4, 3, 5 };
         TestUtils.assertEquals(correctRanks, ranks, 0d);
         ranks = ranking.rank(tiesLast);
-        correctRanks = new double[] { 3, 4, 2, 1 };
+        correctRanks = new double[] { 3, 3, 2, 1 };
         TestUtils.assertEquals(correctRanks, ranks, 0d);
         ranks = ranking.rank(multipleNaNs);
         correctRanks = new double[] { 1, 2, Double.NaN, Double.NaN };
         TestUtils.assertEquals(correctRanks, ranks, 0d);
         ranks = ranking.rank(multipleTies);
-        correctRanks = new double[] { 3, 2, 5, 5, 7, 6, 1 };
+        correctRanks = new double[] { 3, 2, 4, 4, 6, 7, 1 };
         TestUtils.assertEquals(correctRanks, ranks, 0d);
         ranks = ranking.rank(allSame);
-        correctRanks = new double[] { 1, 3, 4, 4 };
+        correctRanks = new double[] { 2, 3, 3, 3 };
         TestUtils.assertEquals(correctRanks, ranks, 0d);
     }