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-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
