[
https://issues.apache.org/jira/browse/MATH-1026?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13774764#comment-13774764
]
Evan Ward commented on MATH-1026:
---------------------------------
Thanks for the review! I think these questions address some important aspects
of the design. Will all the interested people be able to see them here, or
should I cross post to the ML?
bq. 1. Enumerate here all the questions which you asked in the code (cf "//
TODO" marks).
GaussNewtonOptimizerWith\{LU,QR\}Test.java: hahn1 leads to a
singular problem with the Gauss-Newton (pre-existing)
AbstractLeastSquaresOptimizerAbstractTest.java: testGetIterations could
pass with 'return 1;'
AbstractLeastSquaresOptimizerAbstractTest.java:
testMoreEstimatedParametersUnsorted [modification] the first two elements of
point were not previously checked
AbstractLeastSquaresOptimizerAbstractTest.java: what is
testInconsistentEquations actually testing?
AbstractLeastSquaresOptimizerAbstractTest.java:
testInconsistentSizes\{1,2\} test code that has moved to LSFactory and
LSBuilder. These tests should be moved as well.
LevenbergMarquardtOptimizerTest.java: testNonInvertible
checking that RMS > 0.6 is vague.
LevenbergMarquardtOptimizerTest.java: why should the last
checkEstimate of testControlParameters fail? It uses 15 evaluations.
LevenbergMarquardtOptimizerTest.java: checkEstimate should check it
got the right answer to catch failures that don't throw exceptions
LevenbergMarquardtOptimizerTest.java: QuadraticProblem is not used
{quote}
2. There seems to be a design problem:
In "OptimizationProblem", the accessor methods are not all on the same
footing; some create fresh ("Incrementor") objects, but the
"ConvergenceChecker" is a reference to an object created externally (and we
discussed that those could carry state).
{quote}
It is a bit strange. After working with the code I think ConvergenceChecker is
a nice, stateless well defined concept and that trajectory logging should have
it's own part of the API, possibly accessed through the solution or exception
context. Are there other use cases for stateful ConvergenceCheckers? We could
have the user pass in a Builder<ConvergenceChecker> to create a new one every
time. If they needed to read data out of it we would still have to reference it
in the solution or exception context. In conclusion, I like using
ConvergenceChecker as a pure input to the problem instead of I/O as we
discussed before.
bq. Actually, I find it strange that the concepts of allowed number of
"evaluations" and "iterations" are part of the "problem" and not the
"optimizer".
When we discussed it earlier on the ML I thought this was the consensus. (I may
be mistaken, there was a lot of discussion :)
In my understanding the use case for evaluation limits is to prevent an
optimization from using too much cpu time. Since cpu time is roughly
(evaluations)*(cost per evaluation) I thought it made sense to set the
evaluation limit when defining the problem since that is probably when the user
will have the most information about the cost of an evaluation. It also fits
the test cases well, which counts as half a user. :)
{quote}
3. The counting is done explicitly in "LevenbergMarquardtOptimizer" whereas
it seems that the "counting option" should be enabled through a call to
"countEvaluations" in "LeastSquaresFactory".
I'd suggest that instead of an "Incrementor" argument, that method be
passed the argument currently used in "AbstractOptimizationProblem".
{quote}
Yes, I forgot to connect the two. The argument could just be the problem and we
can use the (new) incrementor returned by
OptimizationProblem.getEvaluationCounter().
{quote}
4. "LeastSquaresProblemImpl": in CM, we avoid using the suffix "Impl".
Either it is intended that several implementations can exist (and those
should bear a name that make their specifics explicit), or there will only ever
be one implementation, and (we concluded that) it is better to use a concrete
(abstract) class rather than an interface.
{quote}
I agree the name is ugly. :( If you have a better idea please suggest it. My
other thought was DefaultLeastSquaresProblem, but that is ugly and even longer.
Since it is not part of the public API hopefully the ugly name won't matter too
much.
bq. 5. I would avoid to "import" inner classes. I think that code is easier to
read when one can readily distinguish where one should look for the definition
of an object.
O.k. I had the same thought when I came back to look at it today.
bq. 6. Your comment in "AbstractEvaluation" says it is designed for extension,
yet the class is package-private.
Yeah, I created that to house the common code between UnweightedEvaluation,
DenseWeightedEvaluation, and hopefully DiagonallyWeightedEvaluation. Since
those are the only implementations of Evaluation (that I can think of) package
private is a good thing to reduce the exposed API. If there is a use case for
user defined evaluations we can make it public.
bq. 7. Class "DenseWeightedEvaluation" has no associated unit tests.
It is tested by EvaluationTest and EvaluationTestValidation. We should add
tests for the other methods as well, especially computeValue(). ;)
I'll make the changes we agreed on (3,5,7) and push/upload a new tarball. For
the other changes I'll wait until we have a consensus.
> Separate Optimization Problem from Algorithm
> --------------------------------------------
>
> Key: MATH-1026
> URL: https://issues.apache.org/jira/browse/MATH-1026
> Project: Commons Math
> Issue Type: Improvement
> Reporter: Evan Ward
> Attachments: opt.tar.gz, opt.tar.gz
>
>
> See discussion on the mailing list starting with:
> http://www.mail-archive.com/[email protected]/msg39681.html
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
