commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles <gil...@harfang.homelinux.org>
Subject Re: [Math] Fluent API, inheritance and immutability
Date Mon, 26 Aug 2013 19:33:26 GMT
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.]

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

>
> 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".

> 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.

> 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.

> 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.

>
> 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...]

>
> 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


Mime
View raw message