luc.maisonobe@free.fr wrote: > The following commit solved MATH-280. Here are some explanations about the change. > > The issue was triggerred by a bracketing attempt in AbstractContinuousDistribution.bracket. This bracketing attempt starts from an initial value (here it was exactly 1.0 since it was mean + standard deviation and we were working on a normalized gaussian). From this initial value, it increases an interval by moving both a lower bound and an upper bound by steps of 1.0, so the intervals used are [0.0, 2.0], [-1.0, 3.0], [-2.0, 4.0] ... In fact, at the first iteration the upper bound (2.0) is exactly the root of the function, so f(b)=0 and the loop ends due to criteria f(a)*f(b) > 0. However the following check was f(a)*f(b) >= 0 and an exception was triggered. > > In fact, there was already a protection against exact solutions in the AbstractContinuousDistribution.inverseCumulativeProbability method. The exception thrown by the bracket method was caught and the domain endpoint were checked to see if they are a solution to the > inverse cumulative computation problem. Here, is was not the case because the solution was an intermediate point (2.0), and the endpoints were 0.0 and Double.MAX_VALUE. So the exception was rethrown ... > Agree with your analysis. I was....slowly...coming to the same conclusion ;) > The solution I used was to have consistent tests in the bracket method: both now use f(a)*f(b) > 0 and the correct result is returned. > > This however have changed the behaviour of the bracket method: it doesn't throw an exception anymore when an exact root is encountered. This seemed a logical choice as there was already some defensive code in the upper level methods and I think we have fixed the solvers one year ago to find root exactly at endpoints (see MATH-204). Looks like only Brent was fixed in MATH-204, but others are OK, except for Secant, which we should probably fix. > This behaviour was used in a test case (testBracketCornerSolution). I have removed this test but feel uncomfortable with this choice. > The contract of the method was changed - and documented - so removing this test case is OK. Probably should replace it with a test confirming success when a root is encountered. > Could someone either acknowledge this choice or propose another fix for the problem ? > I am +1 on the changes. Since the release notes will be generated from changes.xml, it is probably best to add some more commentary there on the contract change for UnivariateSolverUtils.bracket. Since solvers are pluggable in the inverse probability distribution computations, it is probably also a good idea to make the following change to the UnivariateRealSolver.solve javadoc: s/A solver may require that the interval brackets a single zero root./A solver may require that the interval brackets a single zero root. Solvers that do require bracketing should be able to handle the case where one of the endpoints is itself a root./ Phil > thanks, > Luc > > ----- luc@apache.org a écrit : > > >> Author: luc >> Date: Tue Jul 7 09:19:46 2009 >> New Revision: 791766 >> >> URL: http://svn.apache.org/viewvc?rev=791766&view=rev >> Log: >> fixed a bracketing issue due to inconsistent checks >> JIRA: MATH-280 >> >> Modified: >> >> commons/proper/math/trunk/src/java/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtils.java >> >> commons/proper/math/trunk/src/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java >> commons/proper/math/trunk/src/site/xdoc/changes.xml >> >> commons/proper/math/trunk/src/test/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtilsTest.java >> >> commons/proper/math/trunk/src/test/org/apache/commons/math/distribution/NormalDistributionTest.java >> >> Modified: >> commons/proper/math/trunk/src/java/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtils.java >> URL: >> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/java/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtils.java?rev=791766&r1=791765&r2=791766&view=diff >> ============================================================================== >> --- >> commons/proper/math/trunk/src/java/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtils.java >> (original) >> +++ >> commons/proper/math/trunk/src/java/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtils.java >> Tue Jul 7 09:19:46 2009 >> @@ -131,7 +131,7 @@ >> /** >> * This method attempts to find two values a and b satisfying >>
>> *
• ` lowerBound <= a < initial < b <= upperBound` >>
• >> - *
• ` f(a) * f(b) < 0 `
• >> + *
• ` f(a) * f(b) <= 0 `
• >> *
>> * If f is continuous on `[a,b],` this means that >> `a` >> * and `b` bracket a root of f. >> @@ -141,7 +141,7 @@ >> * function at `a` and `b` and keeps >> moving >> * the endpoints out by one unit each time through a loop that >> terminates >> * when one of the following happens:
>> - *
• ` f(a) * f(b) < 0 ` -- success!
• >> + *
• ` f(a) * f(b) <= 0 ` -- success!
• >> *
• ` a = lower ` and ` b = upper` >> * -- ConvergenceException
• >> *
• ` maximumIterations` iterations elapse >> @@ -195,7 +195,7 @@ >> } while ((fa * fb > 0.0) && (numIterations < >> maximumIterations) && >> ((a > lowerBound) || (b < upperBound))); >> >> - if (fa * fb >= 0.0 ) { >> + if (fa * fb > 0.0 ) { >> throw new ConvergenceException( >> "number of iterations={0}, maximum >> iterations={1}, " + >> "initial={2}, lower bound={3}, upper bound={4}, >> final a value={5}, " + >> >> Modified: >> commons/proper/math/trunk/src/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java >> URL: >> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java?rev=791766&r1=791765&r2=791766&view=diff >> ============================================================================== >> --- >> commons/proper/math/trunk/src/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java >> (original) >> +++ >> commons/proper/math/trunk/src/java/org/apache/commons/math/distribution/AbstractContinuousDistribution.java >> Tue Jul 7 09:19:46 2009 >> @@ -65,7 +65,7 @@ >> } >> >> // by default, do simple root finding using bracketing and >> default solver. >> - // subclasses can overide if there is a better method. >> + // subclasses can override if there is a better method. >> UnivariateRealFunction rootFindingFunction = >> new UnivariateRealFunction() { >> public double value(double x) throws >> FunctionEvaluationException { >> >> Modified: commons/proper/math/trunk/src/site/xdoc/changes.xml >> URL: >> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/site/xdoc/changes.xml?rev=791766&r1=791765&r2=791766&view=diff >> ============================================================================== >> --- commons/proper/math/trunk/src/site/xdoc/changes.xml (original) >> +++ commons/proper/math/trunk/src/site/xdoc/changes.xml Tue Jul 7 >> 09:19:46 2009 >> @@ -39,6 +39,9 @@ >> >> >> >> + >> + Fixed a bracketing issue in inverse cumulative probability >> computation >> + >> >> Added a check for too few rows with respect to the number of >> predictors in linear regression >> >> >> Modified: >> commons/proper/math/trunk/src/test/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtilsTest.java >> URL: >> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/test/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtilsTest.java?rev=791766&r1=791765&r2=791766&view=diff >> ============================================================================== >> --- >> commons/proper/math/trunk/src/test/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtilsTest.java >> (original) >> +++ >> commons/proper/math/trunk/src/test/org/apache/commons/math/analysis/solvers/UnivariateRealSolverUtilsTest.java >> Tue Jul 7 09:19:46 2009 >> @@ -17,13 +17,12 @@ >> >> package org.apache.commons.math.analysis.solvers; >> >> -import org.apache.commons.math.ConvergenceException; >> +import junit.framework.TestCase; >> + >> import org.apache.commons.math.MathException; >> import org.apache.commons.math.analysis.SinFunction; >> import org.apache.commons.math.analysis.UnivariateRealFunction; >> >> -import junit.framework.TestCase; >> - >> /** >> * @version \$Revision\$ \$Date\$ >> */ >> @@ -91,15 +90,6 @@ >> assertTrue(sin.value(result[1]) > 0); >> } >> >> - public void testBracketCornerSolution() throws MathException { >> - try { >> - UnivariateRealSolverUtils.bracket(sin, 1.5, 0, 2.0); >> - fail("Expecting ConvergenceException"); >> - } catch (ConvergenceException ex) { >> - // expected >> - } >> - } >> - >> public void testBadParameters() throws MathException { >> try { // null function >> UnivariateRealSolverUtils.bracket(null, 1.5, 0, 2.0); >> >> Modified: >> commons/proper/math/trunk/src/test/org/apache/commons/math/distribution/NormalDistributionTest.java >> URL: >> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/test/org/apache/commons/math/distribution/NormalDistributionTest.java?rev=791766&r1=791765&r2=791766&view=diff >> ============================================================================== >> --- >> commons/proper/math/trunk/src/test/org/apache/commons/math/distribution/NormalDistributionTest.java >> (original) >> +++ >> commons/proper/math/trunk/src/test/org/apache/commons/math/distribution/NormalDistributionTest.java >> Tue Jul 7 09:19:46 2009 >> @@ -17,6 +17,8 @@ >> >> package org.apache.commons.math.distribution; >> >> +import org.apache.commons.math.MathException; >> + >> /** >> * Test cases for NormalDistribution. >> * Extends ContinuousDistributionAbstractTest. See class javadoc >> for >> @@ -161,4 +163,11 @@ >> } >> } >> } >> + >> + public void testMath280() throws MathException { >> + NormalDistribution normal = new NormalDistributionImpl(0,1); >> + double result = >> normal.inverseCumulativeProbability(0.9772498680518209); >> + assertEquals(2.0, result, 1.0e-12); >> + } >> + >> } >> > > --------------------------------------------------------------------- > 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