[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12934224#action_12934224
]
Gilles commented on MATH-439:
-----------------------------
Coming back to the failing {{testCircleFitting}} test in
{{NonLinearConjugateGradientOptimizerTest}}.
In the {{NonLinearConjugateGradientOptimizer}} class (around line 160), I've
replaced the line
{code}
final double step = solver.solve(lsf, 0, findUpperBound(lsf, 0, initialStep));
{code}
with
{code}
final double uB = findUpperBound(lsf, 0, initialStep);
final double step = solver.solve(lsf, 0, uB);
// final double step = solver.solve(lsf, 0, uB, 0.5 * uB);
{code}
As it is above, the code is equivalent to the first excerpt, and the test
succeeds.
Now, if we replace the second line by the third (in order to use the 3
parameters {{solve}} method), the test fails!
This is all done in the current trunk version (_without_ any of the refactored
code). So there is indeed a bug but not one which I would have introduced with
the refactoring, as I first thought.
The bug was silent because in {{BrentSolver}}, the code (line 231) copies "x2"
(itself being a copy of the lower bound "x0", cf. line 124 or 136) into "x1"
(which becomes 0 as can be seen in the above code); thus the selected "step"
value is 0. But it is only by chance, because when {{BrentSolver}} returns this
value, _all_ the values in the search interval are below the "functionAccuracy"
threshold. And indeed, in the 3-parameters version of {{solve}}, it is the
"initial" value which is tested first (line 110) and is thus returned as the
root. But in that case the step is not zero and somehow the NLCG algorithm
starts to diverge...
Can you confirm the problem?
> Refactoring of solvers (package "analysis.solvers")
> ---------------------------------------------------
>
> Key: MATH-439
> URL: https://issues.apache.org/jira/browse/MATH-439
> Project: Commons Math
> Issue Type: Improvement
> Reporter: Gilles
> Priority: Minor
> Fix For: 3.0
>
> Attachments: AbstractUnivariateRealSolver.java
>
>
> The classes in package "analysis.solvers" could be refactored similarly to
> what was done for package {{optimization}}.
> * Replace {{MaxIterationsExceededException}} with
> {{TooManyEvaluationsException}}:
> Apart from the class {{MaxIterationsExceededException}} being deprecated,
> this approach makes it difficult to compare different algorithms: While the
> concept of iteration is algorithm-dependent, the user is probably mostly
> interested in the number of function evaluations.
> * Implement the method {{solve}} in the base class
> ({{UnivariateRealSolverImpl}}) and define an abstract method {{doSolve}} to
> be implemented in derived classes. This method would then use a new
> {{computeObjectiveFunction}} method that will take care of the counting of
> the function evaluations.
> * Remove "protected" fields (the root is unnecessary since it is returned by
> {{solve}}). Arguingly the function value is also not very useful (as we know
> what it should be), except for debugging purposes (in which case, it might
> not be a problem to call the function's {{value}} method once more).
> * Remove the tolerance setter (accuracy) and make the corresponding fields
> "final".
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
