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. [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). > 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. 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
