scalanlp / breeze

Breeze is a numerical processing library for Scala.
www.scalanlp.org
Apache License 2.0
3.44k stars 691 forks source link

Bugs in breeze.stats.distributions.RandBasis.uniform and randLong #828

Closed Sanzo-Miyazawa closed 2 years ago

Sanzo-Miyazawa commented 2 years ago
  1. The comments of breeze.stats.distributions.RandBasis.uniform indicate "Uniformly samples in [0,1]". However, I think that this comment should be corrected as "[0, 1)", because the java code finally called, org.apache.commons.math3.random.MersenneTwister.nextDouble == org.apache.commons.math3.random.BitsStreamGenerator.nextDouble , will generate [0, 1).

    public double nextDouble() {
        final long high = ((long) next(26)) << 26;
        final int  low  = next(26);
        return (high | low) * 0x1.0p-52d;
    }

    If so, "require( low <= high)" in the breeze.stats.distributions.Uniform(low, high) should be "require( low < high)".

  2. Also, I think that the following codes for randLong are not correct.

    /**
    * Uniformly samples a long integer in [0,n)
    */
    def randLong(n: Long): Rand[Long] = new Rand[Long] {
     def draw() = {
      val value = generator.nextLong & Long.MaxValue
      value % n
    }
    }
    
    /**
    * Uniformly samples a long integer in [n,m)
    */
    def randLong(n: Long, m: Long): Rand[Long] = new Rand[Long] {
    def draw() = {
      val value = generator.nextLong & Long.MaxValue
      value % (m - n) + n
    }
    }

    They may be written as

    
    /**
    * Uniformly samples a long integer in [0,n)
    */
    def randLong(n: Long): Rand[Long] = new Rand[Long] {
    require(n > 0, "n > 0")
    def draw() = {
      val maxVal = Long.MaxValue - (Long.MaxValue % n) - 1
      var value = (generator.nextLong() & Long.MaxValue)
      while ( value > maxVal ) {
        value = (generator.nextLong() & Long.MaxValue)
      }
      value % n
    }
    }

/**