commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles Sadowski <gil...@harfang.homelinux.org>
Subject Re: [MATH] Test failure in Continuum
Date Sun, 05 Aug 2012 22:49:44 GMT
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: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message