On 12/14/12 12:58 PM, Gilles Sadowski wrote: > On Fri, Dec 14, 2012 at 12:14:45PM -0800, Phil Steitz wrote: >> On 12/14/12 11:59 AM, Gilles Sadowski wrote: >>> On Fri, Dec 14, 2012 at 10:03:14AM -0800, Phil Steitz wrote: >>>> On 12/14/12 9:43 AM, Phil Steitz wrote: >>>>> On 12/14/12 8:52 AM, Gilles Sadowski wrote: >>>>>> On Fri, Dec 14, 2012 at 04:28:25PM -0000, pste...@apache.org wrote: >>>>>>> Author: psteitz >>>>>>> Date: Fri Dec 14 16:28:23 2012 >>>>>>> New Revision: 1421968 >>>>>>> >>>>>>> URL: http://svn.apache.org/viewvc?rev=1421968&view=rev >>>>>>> Log: >>>>>>> Reverted incompatible changes made in r1420006. >>>>>> I don't understand why you did that. >>>>>> You nullified my changes that fixed (IIUC) the problem while still >>>>>> providing users a smooth upgrade path to using RandomDataGenerator >>>>>> (which it seems was your goal). >>>>> I was basically reverting my original commit, which caused the >>>>> problem. I started with a revert back to before r1420006. Then I >>>>> just added some deprecations. I should not have introduced new >>>>> constructors or RandomDataGenerator at all at this point. >>>> Sorry. Misspoke there. What I meant was I should not have >>>> introduced a constructor using RandomDataGenerator. I *did* >>>> introduce one using RandomGenerator, which is what should be used. >>>> Sorry for the bad communication and somewhat convoluted naming on >>>> this. Hopefully this makes sense. >>> Si, ultimately, there will be neither a "RandomDataImpl" nor a >>> "RandomDataGenerator" (as I suggested in the comments to MATH-915)? >> Right, there will be no *constructor* taking one of those. There >> will be a member variable, constructed from a RandomGenerator, which >> will change from being a RandomDataImpl to a RandomDataGenerator. > I don't understand why "EmpiricalDistribution" would need a > "RandomDataGenerator" at all. That duplicates the "random source" readily > available from the parent class.
RandomDataGenerator extends the capabilities of RandomGenerator and EmpiricalDistribution uses the extended capabilities. > [That was part of my comment to MATH-915.] > The "randomData" field is only actually used in "getNextValue" where the > code could directly use "UniformRealDistribution" and "NormalDistribution" > variables instead (as my overwritten fix was doing). Your version was using RandomDataGenerator / RandomDataImpl instances to do the work. In theory, you could use distribution instances to do this; but this is precisely the kind of use case that RandomDataImpl was created to address - direct access to data generation methods following common parametric distributions. > >> We could make that change now if RandomDataImpl exposed a >> getDelegate method to deliver the wrapped RandomDataGenerator. > It does. That was one of my changes to fix MATH-916. Great. We think alike (sometimes :) Given that, we could s/RandomDataImpl/RandomDataGenerator in the private field. Phil > > Gilles > >> Then the current member variable could be changed to a >> RandomDataGenerator and the (now deprecated) constructor taking a >> RandomDataImpl could convert it. That is a little more work to >> implement and probably not worth the hassle for 3.1 as the member >> field is private. >> >> Phil >>> Gilles >>> >>>> Phil >>>>> I am >>>>> sorry I did a bad job explaining what was going on there. >>>>> Basically, EmpiricalDistibution needs a RandomDataGenerator / >>>>> RandomDataImpl to generate data following certain distributions. >>>>> What should be provided as a constructor argument for this class >>>>> (and ValueServer) is a RandomGenerator, which is the underlying >>>>> source of random data. The RandomDataImpl constructors are really >>>>> legacy, going back to the days before RandomGenerator existed. Does >>>>> this make sense? >>>>> >>>>> Phil >>>>>> Gilles >>>>>> >>>>>>> Fixed javadoc error in EmpiricalDistribution class javadoc. >>>>>>> Deprecated constructors taking RandomDataImpl instances in >>>>>>> EmpiricalDistribution, ValueServer. These constructors predate >>>>>>> RandomGenerator, which should be used directly as the source of random >>>>>>> data for these classes. >>>>>>> >>>>>>> Modified: >>>>>>> >>>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/EmpiricalDistribution.java >>>>>>> >>>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/ValueServer.java >>>>>>> >>>>>>> Modified: >>>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/EmpiricalDistribution.java >>>>>>> URL: >>>>>>> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/EmpiricalDistribution.java?rev=1421968&r1=1421967&r2=1421968&view=diff >>>>>>> ============================================================================== >>>>>>> --- >>>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/EmpiricalDistribution.java >>>>>>> (original) >>>>>>> +++ >>>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/EmpiricalDistribution.java >>>>>>> Fri Dec 14 16:28:23 2012 >>>>>>> @@ -29,8 +29,8 @@ import java.util.ArrayList; >>>>>>> import java.util.List; >>>>>>> >>>>>>> import org.apache.commons.math3.distribution.AbstractRealDistribution; >>>>>>> -import org.apache.commons.math3.distribution.RealDistribution; >>>>>>> import org.apache.commons.math3.distribution.NormalDistribution; >>>>>>> +import org.apache.commons.math3.distribution.RealDistribution; >>>>>>> import org.apache.commons.math3.exception.MathIllegalStateException; >>>>>>> import org.apache.commons.math3.exception.MathInternalError; >>>>>>> import org.apache.commons.math3.exception.NullArgumentException; >>>>>>> @@ -134,18 +134,14 @@ public class EmpiricalDistribution exten >>>>>>> /** upper bounds of subintervals in (0,1) "belonging" to the bins >>>>>>> */ >>>>>>> private double[] upperBounds = null; >>>>>>> >>>>>>> - /** Data generator. */ >>>>>>> - private final RandomDataGenerator randomDataGen; >>>>>>> - /** >>>>>>> - * XXX Enable backward-compatibility (to be removed in 4.0). >>>>>>> - */ >>>>>>> - private final boolean useRandomDataImpl; >>>>>>> + /** RandomDataImpl instance to use in repeated calls to getNext() >>>>>>> */ >>>>>>> + private final RandomDataImpl randomData; >>>>>>> >>>>>>> /** >>>>>>> * Creates a new EmpiricalDistribution with the default bin count. >>>>>>> */ >>>>>>> public EmpiricalDistribution() { >>>>>>> - this(DEFAULT_BIN_COUNT); >>>>>>> + this(DEFAULT_BIN_COUNT, new RandomDataImpl()); >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> @@ -154,7 +150,7 @@ public class EmpiricalDistribution exten >>>>>>> * @param binCount number of bins >>>>>>> */ >>>>>>> public EmpiricalDistribution(int binCount) { >>>>>>> - this(binCount, (RandomGenerator) null); >>>>>>> + this(binCount, new RandomDataImpl()); >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> @@ -162,82 +158,52 @@ public class EmpiricalDistribution exten >>>>>>> * provided {@link RandomGenerator} as the source of random data. >>>>>>> * >>>>>>> * @param binCount number of bins >>>>>>> - * @param randomData random data generator (may be null, resulting >>>>>>> in a default generator) >>>>>>> - * @deprecated As of 3.1. To be removed in 4.0. Please use >>>>>>> - * {@link #EmpiricalDistribution(int,RandomDataGenerator)} instead. >>>>>>> + * @param generator random data generator (may be null, resulting >>>>>>> in default JDK generator) >>>>>>> + * @since 3.0 >>>>>>> */ >>>>>>> - @Deprecated >>>>>>> - public EmpiricalDistribution(int binCount, RandomDataImpl >>>>>>> randomData) { >>>>>>> + public EmpiricalDistribution(int binCount, RandomGenerator >>>>>>> generator) { >>>>>>> this.binCount = binCount; >>>>>>> - this.randomData = randomData == null ? >>>>>>> - new RandomDataImpl() : >>>>>>> - randomData; >>>>>>> + randomData = new RandomDataImpl(generator); >>>>>>> binStats = new ArrayList<SummaryStatistics>(); >>>>>>> - useRandomDataImpl = true; >>>>>>> - randomDataGen = null; >>>>>>> - } >>>>>>> - /** >>>>>>> - * Creates a new EmpiricalDistribution with the specified bin >>>>>>> count using the >>>>>>> - * provided {@link RandomGenerator} as the source of random data. >>>>>>> - * >>>>>>> - * @param randomData random data generator (may be null, resulting >>>>>>> in a default generator) >>>>>>> - * @deprecated As of 3.1. To be removed in 4.0. Please use >>>>>>> - * {@link #EmpiricalDistribution(RandomDataGenerator)} instead. >>>>>>> - */ >>>>>>> - @Deprecated >>>>>>> - public EmpiricalDistribution(RandomDataImpl randomData) { >>>>>>> - this(DEFAULT_BIN_COUNT, randomData); >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> - * Creates a new EmpiricalDistribution with the specified bin >>>>>>> count using the >>>>>>> - * provided {@link RandomGenerator} as the source of random data. >>>>>>> - * >>>>>>> - * @param binCount number of bins >>>>>>> - * @param randomData random data generator (may be null, resulting >>>>>>> in a default generator) >>>>>>> - */ >>>>>>> - public EmpiricalDistribution(int binCount, RandomDataGenerator >>>>>>> randomData) { >>>>>>> - this.binCount = binCount; >>>>>>> - this.randomDataGen = randomData == null ? >>>>>>> - new RandomDataGenerator() : >>>>>>> - randomData; >>>>>>> - binStats = new ArrayList<SummaryStatistics>(); >>>>>>> - useRandomDataImpl = false; // XXX Remove in 4.0 >>>>>>> - } >>>>>>> - /** >>>>>>> - * Creates a new EmpiricalDistribution with the specified bin >>>>>>> count using the >>>>>>> + * Creates a new EmpiricalDistribution with default bin count >>>>>>> using the >>>>>>> * provided {@link RandomGenerator} as the source of random data. >>>>>>> * >>>>>>> - * @param randomData random data generator (may be null, resulting >>>>>>> in a default generator) >>>>>>> + * @param generator random data generator (may be null, resulting >>>>>>> in default JDK generator) >>>>>>> + * @since 3.0 >>>>>>> */ >>>>>>> - public EmpiricalDistribution(RandomDataGenerator randomData) { >>>>>>> - this(DEFAULT_BIN_COUNT, randomData); >>>>>>> + public EmpiricalDistribution(RandomGenerator generator) { >>>>>>> + this(DEFAULT_BIN_COUNT, generator); >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> * Creates a new EmpiricalDistribution with the specified bin >>>>>>> count using the >>>>>>> - * provided {@link RandomGenerator} as the source of random data. >>>>>>> + * provided {@link RandomDataImpl} instance as the source of >>>>>>> random data. >>>>>>> * >>>>>>> * @param binCount number of bins >>>>>>> - * @param generator random data generator (may be null, resulting >>>>>>> in a default generator) >>>>>>> + * @param randomData random data generator (may be null, resulting >>>>>>> in default JDK generator) >>>>>>> * @since 3.0 >>>>>>> */ >>>>>>> - public EmpiricalDistribution(int binCount, RandomGenerator >>>>>>> generator) { >>>>>>> - this(binCount, new RandomDataGenerator(generator)); >>>>>>> + public EmpiricalDistribution(int binCount, RandomDataImpl >>>>>>> randomData) { >>>>>>> + this.binCount = binCount; >>>>>>> + this.randomData = randomData; >>>>>>> + binStats = new ArrayList<SummaryStatistics>(); >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> * Creates a new EmpiricalDistribution with default bin count >>>>>>> using the >>>>>>> - * provided {@link RandomGenerator} as the source of random data. >>>>>>> + * provided {@link RandomDataImpl} as the source of random data. >>>>>>> * >>>>>>> - * @param generator random data generator (may be null, resulting >>>>>>> in default generator) >>>>>>> + * @param randomData random data generator (may be null, resulting >>>>>>> in default JDK generator) >>>>>>> * @since 3.0 >>>>>>> */ >>>>>>> - public EmpiricalDistribution(RandomGenerator generator) { >>>>>>> - this(DEFAULT_BIN_COUNT, generator); >>>>>>> + public EmpiricalDistribution(RandomDataImpl randomData) { >>>>>>> + this(DEFAULT_BIN_COUNT, randomData); >>>>>>> } >>>>>>> >>>>>>> - /** >>>>>>> + /** >>>>>>> * Computes the empirical distribution from the provided >>>>>>> * array of numbers. >>>>>>> * >>>>>>> @@ -288,7 +254,7 @@ public class EmpiricalDistribution exten >>>>>>> } finally { >>>>>>> try { >>>>>>> in.close(); >>>>>>> - } catch (IOException ex) { // NOPMD >>>>>>> + } catch (IOException ex) { >>>>>>> // ignore >>>>>>> } >>>>>>> } >>>>>>> @@ -320,7 +286,7 @@ public class EmpiricalDistribution exten >>>>>>> } finally { >>>>>>> try { >>>>>>> in.close(); >>>>>>> - } catch (IOException ex) { // NOPMD >>>>>>> + } catch (IOException ex) { >>>>>>> // ignore >>>>>>> } >>>>>>> } >>>>>>> @@ -497,41 +463,22 @@ public class EmpiricalDistribution exten >>>>>>> throw new >>>>>>> MathIllegalStateException(LocalizedFormats.DISTRIBUTION_NOT_LOADED); >>>>>>> } >>>>>>> >>>>>>> - if (useRandomDataImpl) { >>>>>>> - // XXX backward compatibility. >>>>>>> - // Start with a uniformly distributed random number in (0, >>>>>>> 1) >>>>>>> - final double x = randomData.nextUniform(0,1); >>>>>>> - // Use this to select the bin and generate a Gaussian >>>>>>> within the bin >>>>>>> - for (int i = 0; i < binCount; i++) { >>>>>>> - if (x <= upperBounds[i]) { >>>>>>> - SummaryStatistics stats = binStats.get(i); >>>>>>> - if (stats.getN() > 0) { >>>>>>> - if (stats.getStandardDeviation() > 0) { // >>>>>>> more than one obs >>>>>>> - return >>>>>>> randomData.nextGaussian(stats.getMean(), >>>>>>> - >>>>>>> stats.getStandardDeviation()); >>>>>>> - } else { >>>>>>> - return stats.getMean(); // only one obs in >>>>>>> bin >>>>>>> - } >>>>>>> - } >>>>>>> - } >>>>>>> - } >>>>>>> - } else { >>>>>>> - // Start with a uniformly distributed random number in (0, >>>>>>> 1) >>>>>>> - final double x = randomDataGen.nextUniform(0, 1); >>>>>>> - // Use this to select the bin and generate a Gaussian >>>>>>> within the bin >>>>>>> - for (int i = 0; i < binCount; i++) { >>>>>>> - if (x <= upperBounds[i]) { >>>>>>> - SummaryStatistics stats = binStats.get(i); >>>>>>> - if (stats.getN() > 0) { >>>>>>> - if (stats.getStandardDeviation() > 0) { // >>>>>>> more than one obs >>>>>>> - return >>>>>>> randomDataGen.nextGaussian(stats.getMean(), >>>>>>> - >>>>>>> stats.getStandardDeviation()); >>>>>>> - } else { >>>>>>> - return stats.getMean(); // only one obs in >>>>>>> bin >>>>>>> - } >>>>>>> - } >>>>>>> - } >>>>>>> - } >>>>>>> + // Start with a uniformly distributed random number in (0,1) >>>>>>> + final double x = randomData.nextUniform(0,1); >>>>>>> + >>>>>>> + // Use this to select the bin and generate a Gaussian within >>>>>>> the bin >>>>>>> + for (int i = 0; i < binCount; i++) { >>>>>>> + if (x <= upperBounds[i]) { >>>>>>> + SummaryStatistics stats = binStats.get(i); >>>>>>> + if (stats.getN() > 0) { >>>>>>> + if (stats.getStandardDeviation() > 0) { // more >>>>>>> than one obs >>>>>>> + return randomData.nextGaussian(stats.getMean(), >>>>>>> + >>>>>>> stats.getStandardDeviation()); >>>>>>> + } else { >>>>>>> + return stats.getMean(); // only one obs in bin >>>>>>> + } >>>>>>> + } >>>>>>> + } >>>>>>> } >>>>>>> throw new >>>>>>> MathIllegalStateException(LocalizedFormats.NO_BIN_SELECTED); >>>>>>> } >>>>>>> @@ -624,12 +571,7 @@ public class EmpiricalDistribution exten >>>>>>> * @since 3.0 >>>>>>> */ >>>>>>> public void reSeed(long seed) { >>>>>>> - if (useRandomDataImpl) { >>>>>>> - // XXX backward compatibility. >>>>>>> - randomData.reSeed(seed); >>>>>>> - } else { >>>>>>> - randomDataGen.reSeed(seed); >>>>>>> - } >>>>>>> + randomData.reSeed(seed); >>>>>>> } >>>>>>> >>>>>>> // Distribution methods --------------------------- >>>>>>> @@ -819,7 +761,7 @@ public class EmpiricalDistribution exten >>>>>>> */ >>>>>>> @Override >>>>>>> public void reseedRandomGenerator(long seed) { >>>>>>> - reSeed(seed); >>>>>>> + randomData.reSeed(seed); >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> >>>>>>> Modified: >>>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/ValueServer.java >>>>>>> URL: >>>>>>> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/ValueServer.java?rev=1421968&r1=1421967&r2=1421968&view=diff >>>>>>> ============================================================================== >>>>>>> --- >>>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/ValueServer.java >>>>>>> (original) >>>>>>> +++ >>>>>>> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random/ValueServer.java >>>>>>> Fri Dec 14 16:28:23 2012 >>>>>>> @@ -88,35 +88,36 @@ public class ValueServer { >>>>>>> private BufferedReader filePointer = null; >>>>>>> >>>>>>> /** RandomDataImpl to use for random data generation. */ >>>>>>> - private final RandomDataGenerator randomData; >>>>>>> + private final RandomDataImpl randomData; >>>>>>> >>>>>>> // Data generation modes ====================================== >>>>>>> >>>>>>> /** Creates new ValueServer */ >>>>>>> public ValueServer() { >>>>>>> - randomData = new RandomDataGenerator(); >>>>>>> + randomData = new RandomDataImpl(); >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> - * Construct a ValueServer instance using a RandomDataGenerator as >>>>>>> its source >>>>>>> + * Construct a ValueServer instance using a RandomDataImpl as its >>>>>>> source >>>>>>> * of random data. >>>>>>> * >>>>>>> - * @param randomData random data source >>>>>>> + * @param randomData the RandomDataImpl instance used to source >>>>>>> random data >>>>>>> * @since 3.0 >>>>>>> + * @deprecated use {@link #ValueServer(RandomGenerator)} >>>>>>> */ >>>>>>> - public ValueServer(RandomDataGenerator randomData) { >>>>>>> + public ValueServer(RandomDataImpl randomData) { >>>>>>> this.randomData = randomData; >>>>>>> } >>>>>>> + >>>>>>> /** >>>>>>> - * Construct a ValueServer instance using a RandomDataImpl as its >>>>>>> source >>>>>>> + * Construct a ValueServer instance using a RandomGenerator as its >>>>>>> source >>>>>>> * of random data. >>>>>>> * >>>>>>> - * @param randomData random data source >>>>>>> - * @deprecated As of 3.1. Use {@link >>>>>>> #ValueServer(RandomDataGenerator)} instead. >>>>>>> + * @since 3.1 >>>>>>> + * @param generator source of random data >>>>>>> */ >>>>>>> - @Deprecated >>>>>>> - public ValueServer(RandomDataImpl randomData) { >>>>>>> - this(randomData.getDelegate()); >>>>>>> + public ValueServer(RandomGenerator generator) { >>>>>>> + this.randomData = new RandomDataImpl(generator); >>>>>>> } >>>>>>> >>>>>>> /** >>>>>>> @@ -288,7 +289,7 @@ public class ValueServer { >>>>>>> try { >>>>>>> filePointer.close(); >>>>>>> filePointer = null; >>>>>>> - } catch (IOException ex) { // NOPMD >>>>>>> + } catch (IOException ex) { >>>>>>> // ignore >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>>>> >>>>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>> For additional commands, e-mail: dev-h...@commons.apache.org >>> >>> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
