commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: svn commit: r1421968 - in /commons/proper/math/trunk/src/main/java/org/apache/commons/math3/random: EmpiricalDistribution.java ValueServer.java
Date Fri, 14 Dec 2012 21:29:44 GMT
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, psteitz@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-unsubscribe@commons.apache.org
>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>
>>>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message