[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12965935#action_12965935
]
Gilles commented on MATH-439:
-
{{UnivariateRealSolverImpl}} removed in revision 1041225.
> 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,
> NLCG.testCircleFitting.txt
>
>
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12935786#action_12935786
]
Gilles commented on MATH-439:
-
Refactoring introduced in revision 1039083.
Please review, as the code changed a lot.
The main change is that the API is uniform (restricted to the "solve" methods).
Thus the functionality of the "LaguerreSolver" class has been reduced (some
code was moved to a private inner class). If the "complex" functionality must
be user-accessible, it should be separate from the "real" solvers; creating a
{{analysis.solvers.complex}} sub-package might be appropriate...
The problem with {{NonLinearConjugateGradientOptimizer}} is not solved yet, but
a workaround (that almost behaves as the previous code) lets the unit test pass.
> 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,
> NLCG.testCircleFitting.txt
>
>
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12934869#action_12934869
]
Gilles commented on MATH-439:
-
I've recoded the algorithm in "BrentSolver" using the "zero" in Brent's book
"Algorithms for minimization without derivatives". I wanted to make sure that
it was not something in there that caused the problem; the code did not look
like in the book and there was no code description in the reference indicated
in the Javadoc.
Unfortunately the {{testCircleFitting}} still fails in
{{NonLinearConjugateGradientOptimizerTest}}.
Also with the new code, the {{testIllConditioned}} fails:
{noformat}
java.lang.AssertionError: expected:<-81.0> but was:<-80.80925624330811>
{noformat}
The tolerance in the "assert" is set to 1e-1, shall I raise it to 2e-1 ?
I'd like to keep the new implementation since it follows more closely the book;
that would make it easier for someone to check it in the future, in case
another problem is encountered that could be blamed on "BrentSolver" if it used
in the underlying code. Is that OK?
So, I'm back to wondering whether the code in
{{NonLinearConjugateGradientOptimizer}} is safe: Could it be that it uses a
solution provided by "BrentSolver" which it shouldn't? I mean: If all the
points in the search interval are equally good, what's the point in calling
{{solve}}?
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12934116#action_12934116
]
Luc Maisonobe commented on MATH-439:
OK, I didn't understood you intended to wrap TooManyEvaluationsException in a
moved IntegrationException.
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12934018#action_12934018
]
Gilles commented on MATH-439:
-
> [...] yes, raising the threshold is an appropriate fix [...]
Fine. Thanks.
> Our exception handling is already quite complex now [...]
Now? I hold that it is much _simpler_ now (or, more exactly: It will be when
the pending tasks are completed):
* A single {{exception}} package, instead of classes interspersed in several
packages.
* More specific (hence, informative) exception types.
* Obvious exceptions hierarchies (rooted at the various "Math...Exception"
classes that themselves inherit from the Java standard exception specific
categories).
* Message factory and exception localization utilities located in package
{{exception.util}}.
* Reduction of the number of message patterns.
* Embryo of exception usage policy (e.g. not propagating
{{NullPointerException}}).
* No checked exceptions :-)
You ask to slow the pace precisely when I propose to keep something from the
previous design, i.e. the already existing {{IntegrationException}} concept
(albeit moving it to the {{exception}} package and integrating it into the new
design, of course).
MATH-195 is a long-running task but it is a single one. We should cross to the
other bank instead of staying in the middle of the river...
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12933714#action_12933714
]
Luc Maisonobe commented on MATH-439:
Our exception handling is already quite complex now, so I'd rather slow the
pace on this and wait for users feedback before going further.
For DormandPrince, yes, raising the threshold is an appropriate fix. This test
belongs to the non-regression kind, it's not a mandatory threshold defined from
earlier precise settings, so updating it when the code changes is often fair.
The problem is that there is no general relationship between the local error
for which we configure a threshold and the global error we get at the end. This
is still a research subject. The test is here mainly as a safety check to avoid
introducing large regression without noticing them.
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12933596#action_12933596
]
Gilles commented on MATH-439:
-
For {{HighamHall54IntegratorTest}}, the type change did it. Thanks!
Side-note: it would indeed be clearer to wrap such low-level exception types
into an exception that is more appropriate to the method being called.
{{TooManyEvaluationsException}} is low-level in _this_ context because the user
doesn't seem to have any control on the cause of the exception he might get.
So for these useful abstractions, I'd create new classes in package
{{exception}}. This makes another use for a "MathRuntimeException" base class
from which those exceptions, as well as {{MathUserException}}, would inherit.
What do you think?
For {{DormandPrince853IntegratorTest}}, the value you requested is
{noformat}
error=5.3722054293651864E-8
{noformat}
Shall I raise the tolerance to 1e-7 ?
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12933532#action_12933532
]
Luc Maisonobe commented on MATH-439:
The HighamHall54IntegratorTest.testEventsNoConvergence test is explicitly set
up to check an exception is thrown. The exception has changed so the test must
be updated. If you look at the end of the test, you will see:
{code}
try {
integ.integrate(pb,
pb.getInitialTime(), pb.getInitialState(),
pb.getFinalTime(), new double[pb.getDimension()]);
fail("an exception should have been thrown");
} catch (IntegratorException ie) {
assertTrue(ie.getCause() != null);
assertTrue(ie.getCause() instanceof ConvergenceException);
}
{code}
So the behavior you see is normal, you have to adapt the test. As the
TooManyEvaluationsException is not wrapped anymore, it should be sufficient to
replace the catch part with:
{code}
} catch (TooManyEvaluationsException tmee) {
// expected
}
{code}
For the Dormand-Prince test, could you replace at line 238 the
{code}
assertTrue(handler.getMaximalValueError() < 5.0e-8);
{code}
with
{code}
assertEquals(0, handler.getMaximalValueError(), 5.0e-8);
{code}
and tell me the value of the error in the failing test ? It may simply be a too
sensitive test that blows up as soon as you slightly change the code.
I don't understand what happens with the non linear conjugate gradient. The
value is completely out of control (a circle radius of 5 billions instead of
69), so the algorithm has clearly diverged. There is probably a bug somewhere
here.
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12933268#action_12933268
]
Gilles commented on MATH-439:
-
And another regression (maybe).
{noformat}
Tests run: 12, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.271 sec <<<
FAILURE!
testCircleFitting(org.apache.commons.math.optimization.general.NonLinearConjugateGradientOptimizerTest)
Time elapsed: 0.012 sec <<< FAILURE!
java.lang.AssertionError: expected:<69.960161753> but was:<5.9097119121754055E9>
at org.junit.Assert.fail(Assert.java:74)
at org.junit.Assert.failNotEquals(Assert.java:448)
at org.junit.Assert.assertEquals(Assert.java:315)
at org.junit.Assert.assertEquals(Assert.java:346)
at
org.apache.commons.math.optimization.general.NonLinearConjugateGradientOptimizerTest.testCircleFitting(NonLinearConjugateGradientOptimizerTest.java:355)
[...]
{noformat}
The problem is at those lines:
{code}
NonLinearConjugateGradientOptimizer optimizer =
new
NonLinearConjugateGradientOptimizer(ConjugateGradientFormula.POLAK_RIBIERE);
optimizer.setMaxEvaluations(100);
optimizer.setConvergenceChecker(new SimpleScalarValueChecker(1.0e-30,
1.0e-30)); // <--- here
UnivariateRealSolver solver = new BrentSolver(1e-15, 1e-13);
optimizer.setLineSearchSolver(solver);
RealPointValuePair optimum =
optimizer.optimize(circle, GoalType.MINIMIZE, new double[] { 98.680, 47.345
});
{code}
Now, when, in the above, the line marked "<--- here" is commented out, the test
passes.
However, if the line is left in but another solver is used (I tried
{{BisectionSolver}} and {{SecantSolver}}), the test also passes!
Does someone have an idea of what could be going on?
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12933211#action_12933211
]
Gilles commented on MATH-439:
-
Another failing test:
{noformat}
testEvents(org.apache.commons.math.ode.nonstiff.DormandPrince853IntegratorTest)
Time elapsed: 0.001 sec <<< FAILURE!
junit.framework.AssertionFailedError: null
at junit.framework.Assert.fail(Assert.java:47)
at junit.framework.Assert.assertTrue(Assert.java:20)
at junit.framework.Assert.assertTrue(Assert.java:27)
at
org.apache.commons.math.ode.nonstiff.DormandPrince853IntegratorTest.testEvents(DormandPrince853IntegratorTest.java:238)
[...]
{noformat}
Line 238 in the test code is:
{code}
assertTrue(handler.getMaximalValueError() < 5.0e-8);
{code}
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12933203#action_12933203
]
Gilles commented on MATH-439:
-
Changes are again cascading to faraway places.
This issue led to changes in {{BrentSolver}} so that the loop is now
evaluation-based (instead iteration-based) and it now throws
{{TooManyEvaluationsException}}.
The {{HighamHall54IntegratorTest}} test fails:
{noformat}
testEventsNoConvergence(org.apache.commons.math.ode.nonstiff.HighamHall54IntegratorTest)
Time elapsed: 0.006 sec <<< ERROR!
org.apache.commons.math.exception.TooManyEvaluationsException: maximal count
(3) exceeded: evaluations
at
org.apache.commons.math.analysis.solvers.BaseAbstractUnivariateRealSolver.incrementEvaluationCount(BaseAbstractUnivariateRealSolver.java:298)
at
org.apache.commons.math.analysis.solvers.BaseAbstractUnivariateRealSolver.computeObjectiveValue(BaseAbstractUnivariateRealSolver.java:158)
at
org.apache.commons.math.analysis.solvers.BrentSolver.solve(BrentSolver.java:203)
at
org.apache.commons.math.analysis.solvers.BrentSolver.doSolve(BrentSolver.java:89)
at
org.apache.commons.math.analysis.solvers.BaseAbstractUnivariateRealSolver.solve(BaseAbstractUnivariateRealSolver.java:195)
at
org.apache.commons.math.analysis.solvers.BaseAbstractUnivariateRealSolver.solve(BaseAbstractUnivariateRealSolver.java:200)
at
org.apache.commons.math.ode.events.EventState.evaluateStep(EventState.java:260)
at
org.apache.commons.math.ode.events.CombinedEventsManager.evaluateStep(CombinedEventsManager.java:149)
at
org.apache.commons.math.ode.nonstiff.EmbeddedRungeKuttaIntegrator.integrate(EmbeddedRungeKuttaIntegrator.java:293)
at
org.apache.commons.math.ode.nonstiff.HighamHall54IntegratorTest.testEventsNoConvergence(HighamHall54IntegratorTest.java:267)
[...]
{noformat}
Because of the interface change in {{BrentSolver}}, the new set of lines
(around line 260) in "EventState.java" are:
{code}
final BrentSolver solver = new BrentSolver(convergence);
solver.setMaxEvaluations(maxIterationCount);
final double root = (ta <= tb) ? solver.solve(f, ta, tb) : solver.solve(f, tb,
ta);
{code}
Obviously, the new exception is thrown here and is never caught. I don't know
what else to change in order to get the same behaviour as before...
Expert advice welcome!
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12932232#action_12932232
]
Gilles commented on MATH-439:
-
I'm not an "interface" fanatic, meaning that everything should have its
{code}
interface Something {
// ...
}
{code}
together with a
{code}
public class SomethingImpl {
// ...
}
{code}
Especially when there is a single best way to implement the "Something".
In that sense, I find the {{UnivariateRealSolveFactory}} and its associated
{{UnivariateRealSolveFactoryImpl}} quite clumsy (the more so that the former is
not even an interface).
However, in the {{UnivariateRealSolver}} case, I find it completely appropriate
that the interface should be an exact image of the full behaviour of the
implementing classes. If not, it has no purpose from a programming design
viewpoint.
For example, in the {{optimization}} package, there is a specialized interface
for optimizers that rely on differentiability of the objective function. It
should be the same here for the {{NewtonSolver}}.
Similarly, there are also separate interfaces for scalar and vectorial
functions; so for the solvers too, we should find a design that clearly
distinguish between real solvers and complex solvers.
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12932149#action_12932149
]
Luc Maisonobe commented on MATH-439:
The specific solve methods, despite not being in the interface, correspond to
additional specialized behavior. This is part of the added value each solver
provides and some users may really depend on them. This means that for example
a user who does have a differentiable function can directly use a Newton solver
and will not use the general interface. This is fine to me.
I'm not sure the factory is still useful now, but have no clear idea on this.
The utils method may be useful for users who don't have stringent needs on the
method and simply want the current best one, even if it changes when new
versions of the library appears. This is of course a completely different use
case than the one explained above for Newton solver.
If an abstract method can be used to share common code for some solvers, then
it is interesting to add it. If not all solvers fit in this designs, then it is
fine to have some solvers not extending the common abstract class and directly
implement the interface in their own specialized way.
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12931957#action_12931957
]
Gilles commented on MATH-439:
-
I'm stuck because some of the classes do not seem to fit together in the same
package.
# {{LaguerreSolver}} has more {{solve}} methods than mandated by the interface
(for dealing with complex numbers).
# {{MullerSolver}} has a {{solve2}} method.
# {{NewtonSolver}} requires a {{DifferentiableUnivariateRealFunction}} argument
(which is a more stringent requirement than mandated by the interface).
Ideas for cleaning this up are welcome...
Other things which I noticed while working on this issue:
* What's the purpose of {{UnivariateRealSolverFactory}}?
* I don't think that the {{solve}} "shortcut" methods in
{{UnivariateRealSolverUtils}} are very safe. I'd rather have the package
documentation provide guidance about which solver to use than relying on some
"default" solver that is subject to change without notice.
> 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
>
>
> 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.
[jira] Commented: (MATH-439) Refactoring of solvers (package "analysis.solvers")
[
https://issues.apache.org/jira/browse/MATH-439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12931398#action_12931398
]
Luc Maisonobe commented on MATH-439:
Sounds good to me.
> 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
>
>
> 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.
