On Sun, Aug 05, 2012 at 12:54:11PM -0700, Phil Steitz wrote:
> On 8/4/12 10:57 AM, Gilles Sadowski wrote:
> > Hello.
> >
> > Referring to this failed test (cf. messages from Continuum):
> > ---CUT---
> > org.apache.commons.math3.exception.NumberIsTooLargeException: lower bound
> > (65) must be strictly less than upper bound (65)
> > at
> > org.apache.commons.math3.distribution.UniformIntegerDistribution.<init>(UniformIntegerDistribution.java:73)
> > at
> > org.apache.commons.math3.distribution.UniformIntegerDistribution.<init>(UniformIntegerDistribution.java:53)
> > at
> > org.apache.commons.math3.stat.descriptive.AggregateSummaryStatisticsTest.generatePartition(AggregateSummaryStatisticsTest.java:275)
> > at
> > org.apache.commons.math3.stat.descriptive.AggregateSummaryStatisticsTest.testAggregationConsistency(AggregateSummaryStatisticsTest.java:89)
> >
> >
> > It is due to a precondition check while creating the
> > "UniformIntegerDistribution" instance:
> > ---CUT---
> > if (lower >= upper) {
> > throw new NumberIsTooLargeException(
> > LocalizedFormats.LOWER_BOUND_NOT_BELOW_UPPER_BOUND,
> > lower, upper, false);
> > }
> > ---CUT---
> >
> > The test referred to above was using this code (before I changed it use a
> > "UniformIntegerDistribution" instance):
> > ---CUT---
> > final int next = (i == 4 || cur == length - 1) ? length - 1 :
> > randomData.nextInt(cur, length - 1);
> > ---CUT---
> >
> > It is now (after the change):
> > ---CUT---
> > final IntegerDistribution partitionPoint = new
> > UniformIntegerDistribution(cur, length - 1);
> > final int next = (i == 4 || cur == length - 1) ? length - 1 :
> > partitionPoint.sample();
> > ---CUT---
> >
> > Thus, AFAIK, the failure did not appear before because there was no
> > precondition enforcement in "nextInt".
> >
> > The question is: Was the code in the test correct (in allowing the same
> > value for both bounds?
> > * In the negative, how to change it?
> > * The affirmative would mean that the precondition check in
> > "UniformIntegerDistribution" should be relaxed to allow equal bounds.
> > Does this make sense?
> > If so, can we change it now, or is it forbidden in order to stay
> > backwards compatible?
>
> Your analysis above is correct. The failure after the change is due
> to the fact that post-change the distribution is instantiated before
> the bounds check. I changed the test to fix this.
Thanks.
> Both the
> randomData nextInt and the UniformIntegerDistribution constructor
> now forbid the degenerate case where there is only one point in the
> domain. In retrospect, I guess it would have probably been better
> to allow this degenerate case. Unfortunately, this would be an
> incompatible change, so will have to wait until 4.0 if we want to do it.
>
> The original code above illustrates the convenience of being able to
> just make direct calls to randomData.nextXxx, which is why this
> class exists ;)
As I wrote in another post, I'm not against the convenience methods. But
IMO, they should be located in a new "DistributionUtils" class.
And we should also find a way to remove the code duplication (in the
distribution's "sample()" method and in the corresponding "next..." method).
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
