commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Evan Ward <evan.w...@nrl.navy.mil>
Subject Re: [Math] Fluent API, inheritance and immutability
Date Mon, 26 Aug 2013 20:35:51 GMT

On 08/26/2013 03:33 PM, Gilles wrote:
> On Mon, 26 Aug 2013 13:59:32 -0400, Evan Ward wrote:
>> Hi again,
>>
>> I rearranged the least squares package and I've posted the results.[1]
>> I've also created a pull request[2] and an associated issue.[3]
>>
>> [1] https://github.com/wardev/commons-math/tree/sepOpt
>> [2] https://github.com/apache/commons-math/pull/1
>> [3] https://issues.apache.org/jira/browse/MATH-1026
>>
>> A summary of what I changed: (See the git log for more details.)
>
> Thanks for the effort!
> Could you attach a patch to the issue page?
> Hmm, actually, there would be so many changes that I don't think it's
> really useful to have a patch.
> Wouldn't it be clearer to create entirely new classes for everything,
> in a new package? [Suggestions for a name?]
> [Then we can do a "manual" diff for selected files to see how things
> have evolved.]

It is a bit much to view all at once. :) I tried to provide detailed
commit messages and the diffs between commits should be more meaningful
after the first one. In the first commit I added/deleted files to fit
the new interfaces and ensured that all the tests pass. I refactored the
tests so that they can be run on multiple configurations of the same
optimizer. Now GN with QR is actually tested.

If you are looking for a place to start reading I suggest
LeastSquareProblem. The rest of the package is focused on implementing
or using that interface. If have any comments about specific sections,
github has a neat ability to comment on a particular line of the source...

>
> Could you please accompany all the comments below with file names and
> line numbers references?

These comments are essentially a digest of the commit log.[1] More
explanation is provided there along with diffs, file names, and line
number - all in a pretty UI.

[1] https://github.com/apache/commons-math/pull/1/commits

>>
>> 1. Separate LeastSquaresProblem from LeastSquaresOptimizer. Update tests
>> to use the new interfaces.
>> 2. Immutable fluent builders for optimizers.
>> 3. Remove weights from the LeastSquaresProblem interface. They are
>> pre-applied to the Jacobian and residuals.
>> 4. Single function for computed values and Jacobian. I'm not 100% on the
>> name. Call it MultivariateVectorMatrixFunction? or
>> MultivariateJacobianFunction? or
>> MultivariateVectorAndMatrixFunctionWithANameLongerThan80Characters? :)
>> 5. Make LevenbergMarquardt thread safe by using explicit temporary
>> parameters instead of instance fields.
>> 6. In GaussNewton change useLU to an Enum
>> 7. Change ConvergenceChecker<PointVectorValuePair> to
>> CovergenceChecker<Evaluation>. Would allow conditions like "RMS changes
>> less than 1e-3".
>> 8. Use [math] linear package in interfaces (RealMatrix and RealVector
>> instead of double[] and double[][])
>>
>> I did not understand some things in the test code:
>>
>> 1. Why is AbstractLeastSquaresOptimizerTestValidation (renamed to
>> EvaluationTestValidation) disabled by default and how do I tell if it
>> passed?
>
> Not sure that the rename is fine here.
> The comments on the test methods explain the purpose; and the result
> (plotting the output) should be examined "manually".

I renamed it to match the subject under test. It can be renamed back.
I'm just not sure what I'm looking for in the output.

>> 2. Other smaller questions about the tests. Theses are in comments in
>> the test package and marked with TODO.
>
> Please ask them here, so that we can arrive to a common answer.

These smaller questions are more of a code review and make much more
sense next to the code. :)

AbstractLeastSquaresOptimizerAbstractTest:
testGetIterations: Use a more specific test? could pass with 'return 1;'
testMoreEstimatedParametersUnsorted: the first two elements of point
were not previously checked
testInconsistentEquations: what is this actually testing?
testInconsistentSizes1: why is this part here? hasn't 'Ix=b' been tested
already?

LevenbergMarquardtOptimizerTest:
testNonInvertible: check that it is a bad fit? Why the extra conditions?
testControlParameters: why should this fail? It uses 15 evaluations.
(mentioned already)
checkEstimate: check it got the right answer when no exception?
QuadraticProblem: why is this here? It is not used.

>
>> 3. LU and QR use a default singularity threshold. Caused test failures
>> when GaussNewton is configured to use QR. (MATH-1024)
>
> You are looking for trouble.
> The simpler route would be to do the conversion firs, ensuring that all
> tests still pass.
> Afterwards, new issues can be raised and resolved on their own.

All tests still pass, and GN+QR is now tested as throughly as GN+LU.

>
>> 4. A single test case in LevenburgMarquardt.testControlParameters() now
>> converges where it used to diverge. It estimates a reasonable rms after
>> 15 iterations. I'm not sure why it used to diverge...
>
> Same here. You might have uncovered a bug, or you may have introduced
> one.
>
>>
>> There are still several items on the TODO list. Code and discussion is
>> appreciated. :)
>>
>> Affecting the interface:
>>
>> 4. Immutable fluent builders for LSProblem?
>> 6. maxIterations and ConvergenceChecker seem to have duplicate
>> functionality. (To stop the algorithm after a certain number of
>> iterations.) Remove the maxIterations parameter and let the
>> ConvergenceCheck handle that job.
>
> No, the functionality is different. [There was a recent issue
> about this.]
> The optimizer is supposed to raise an exception when the iteration
> count is exhausted, while the "ConvergenceChecker" can be used to
> return the "best solution" found so far.

OK

>
>>
>> Implementation only:
>>
>> 2. Efficient diagonal weights. (don't need to create a new matrix, just
>> multiply in place)
>> 3. Add more convenience methods to Factory and Builder. E.g. for
>> diagonal weights, ...
>> 4. Add conditional logic to Factory so it can use efficient diagonal
>> weights even if they are provided in a matrix.
>> 3. Use [math] linear package in optimizer implementations. To match the
>> current code in performance I think this would mean adding "views" or
>> specialized multiplication methods to the linear package. Depends on
>> MATH-765 and MATH-928.
>
> Use of about-to-be-deprecated code in brand-new code might not be the
> best move. [Even if the deprecation is not imminent...]

Ok, I didn't change the implementation of GN or LM so the
implementations could be discussed later, perhaps after the revision to
the linear package. I think using the linear package for the
implementation of the optimization package will make both packages better.

>
>>
>> Thanks for looking over the proposal and reviewing it.
>>
>
> Thanks for taking the time to delve into the code,
> Gilles
>
>
> ---------------------------------------------------------------------
> 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


Mime
View raw message