Re: [eigen] New Levenberg Marquardt stuff

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

Hi,

sorry my brain is overflowing these days. Clearly we've been too fast on that move. Let's look at the bright side first:

- support for sparse Jacobian in LM

- clear goal to make the LM solver officially part of Eigen.

To complete these goals it is clear to me that we need 1) significant API changes because the current API does not really match the rest of Eigen, and 2) as much as code simplification as possible.

Regarding the API, don't take it the wrong way, we definitely want to heard about you, and I'll come with a proposal soon.

Regarding code simplification this means removing the "memory efficient" path which represent a lot of redundant code and sounds less important on todays computer especially since we support sparse Jacobian: if a dense Jacobian is too large to fit in todays computer memory, I have a bad feeling about the time the solver will take to converge. In any case, if you think the opposite, this feature should be implemented in another class since the associated functor API is different.

Right now, to "fix" the current bad state of the repo I propose the following actions:

- Revert the name changes in NonLinearOptimisation module. There is actually no need to rename the original class.

- Fix licence headers

- Move the new module into unsupported until we agreed on the API

Regarding the module granularity, "NonLinearOptimisation" is way too large. The proposed LevenbergMarquart is perhaps too narrow and I'm just thinking about "NonLinearLeastSquares" for which, as far as know, there is not many standard methods.

gael

On Fri, Dec 7, 2012 at 6:01 PM, Christoph Hertzberg <chtz@xxxxxxxxxxxxxxxxxxxxxxxx> wrote:

Hi,

I just wanted to say that I generally agree.

Most of all, I would say that there should be no commit breaking the API without previous discussion, or if its trivial enough, at least an annunciation.
Furthermore, I would appreciate if things don't get moved from unsupported to supported without giving the possibility to discuss it. This would be the last "clean" moment to bring forward concerns about the module's API or other issues which might exist.
If there are no objections, it should be moved (not copied) 'as is' from the unsupported branch, i.e. either (preferably) do necessary modifications before that or afterwards (of course modifications over time will and shall happen).

Just my 0.02€
Christoph

On 07.12.2012 16:35, Thomas Capricelli wrote:

Hi,

I noticed a lot of huge changes on the LM stuff, and i'm worried.

1) i'm the original author and i haven't heard about this before it was
commited
2) i couldn't see any discussion about it on the mailing list neither
3) I disagree with the changes
     * there's code duplication
     * the move LevenbergMarquardt -> LevenbergMarquardtLegacy breaks
heavily existing code
     * MPL headers were added to those files, which is wrong, they are
under another licence (MINPACK). This is especially bad as i'm
mentionned as the author, i dont back those changes
4) I disagree with the way it was done : the new files are commited as a
whole in one commit. This is really not great, because you can not see
the modification done to the original files. You need to do it by hand.
Mercurial was not used to do the copy.
An ideal way would have been to first commit the 'hg cp xx yy' stuff,
then apply your changes (which are a lot), ideally in small chunks.

well..i really have a bad feeling about this.

Thomas

--

----------------------------------------------
Dipl.-Inf. Christoph Hertzberg
Cartesium 0.049
Universität Bremen
Enrique-Schmidt-Straße 5
28359 Bremen

Tel: +49 (421) 218-64252
----------------------------------------------