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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org