On Fri, Dec 14, 2012 at 01:29:44PM -0800, Phil Steitz wrote: > 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.
I'll commit that in a few minutes... Gilles > > 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 > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
