Re: [eigen] port of (c)minpack to eigen : status

[Thomas Capricelli <orzel@xxxxxxxxxxxxxxx>]

In data giovedì 24 settembre 2009 21:52:02, Benoit Jacob ha scritto:
> First of all, as Hauke noted, there are public data members, I don't

mm, ok, i'll have a look at this! and use the m_* convention as well.

> Speaking of coding style, the indentation rules vary from place to
> place, sometimes it's Eigen-ish, sometimes not at all. Obviously I'd
> ask, please to what you can to Eigen-ify that, but again I don't want
> to put a big burden on you as part of that code comes from a third
> party.

My answer to this is an answer to several other remarks as well. All this code 
is a port from cminpack. But not everything is ported. The high-level stuff is 
'ported' (it is c++, uses as much eigen stuff as possible, remove loops, and 
so on). But the internal are not. For example the qr stuff is not ported 
because i intend to remove it at the end. so no need to care about eigenize it 
now.

Those are considered 'ported' : 
HybridNonLinearSolver.h  LevenbergMarquardt.h  lmpar.h  chkder.h
dogleg.h  covar.h  

Those are not considered 'ported' :

fdjac1.h : compute forward-diff for n*n jacobian, handle banded cases
fdjac2.h : compute forward-diff for m*n jacobian, does NOT handle banded cases
qform.h : compute Q from qr factorization
qrfac.h : col-pivoting householder qr factorization, provided as 'compact' 
form, in the same way as eigen, but not using the exact same representation 
though

qrsolv.h : solve Ax=b using qr factorization, or least-square solution if 
system is not full-rank
r1mpyq.h r1updt.h rwupdt.h : those methods provide (i think) update to the qr 
factorization, using givens rotation. It seems they mix householder and givens 
here...

All this eigenization / indentation is pretty trivial, especially thanks to 
unit tests, so i'm not really bored about those. Also i'm sure lot of people 
would be happy to help there. I'm really more into the API stuff and other 
internal changes such as using eigen qr decompozition.


> The code seems to be only for double, right? Is there a good reason to
> forbid float and other types?

On the contrary it should not, why do you say so ? The tests are only using 
double, cf next answer. The code is using the template parameter 'Scalar' 
everywhere (or should, anyway)

> are very few comments in the code. For example, you mentioned that
> it's hard to reuse the ColPivotingHouseholderQR here, i would like to
> hear about the specifics so I can try to adapt it. Is it in ei_r1updt?
> It's hard to understand what this function does exactly.

you can find documentation at the bottom of 
http://devernay.free.fr/hacks/cminpack.html (pdf)
It's hard because they splitted the qr stuff in several files and i'm not sure 
how to adapt those using eigen. This is mostly a problem of me understanding 
cminpack, not eigen's fault. The files are qform, qfac, qsolve, r1mpyq.h 
r1updt.h rwupdt.h


> At some places there is question of "superdiagonals" and
> "subdiagonals". I don't understand this code, but if this has to do
> with a band matrix, have you investigated the new BandMatrix class?

This is only done in the forward-differentiation, which needs to be done 
differently anyway.

> There are tons of 'for' loops that can and should be replaced by Eigen
> expressions, but that's not a blocker, can be finished later.

indeed. see my previous answer on the topic :-)

> The prerequisite for this is to make very extensive unit tests, so one
> is sure not to introduce regressions.

i agree. i've tried to make a lot because of this.

> By the way, your unit test looks rather good. I just don't know how
> extensive it is (but at least it's big!). It's also funny that it
> works on prescribed instead of random matrices, i guess that comes
> from minpack..?

there are two kinds of tests : those coming from cminpack examples, and those 
coming from the nist set. See my original mail for a description of those.

(from second mail)
> I guess I saw a comment mentioning that the ei_... functions should
> become member methods; obviously I support that; i'd like to mention
> that this would allow you to get rid of the wordy
> 
>  Matrix<Scalar, Dynamic, Dynamic>
> 
> etc... that you have to do all the time; instead you could have a
> typedef "Matrix" for that, once and for all.

I intend to do that, but i first need all internals to be ported/uniform, 
which they are not yet.

thanks for your feedback!,
Thomas
-- 
Thomas Capricelli <orzel@xxxxxxxxxxxxxxx>
http://www.freehackers.org/thomas