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

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

Ah yes, another remark.

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.

Benoit

2009/9/24 Benoit Jacob <jacob.benoit.1@xxxxxxxxx>:
> Ok, me too, a few remarks...
>
> First of all, as Hauke noted, there are public data members, I don't
> think that's a good idea. It's true that, being a pure headers
> library, we can't magically solve ABI compatibility problems by
> protecting data members. But still, there are other advantages in
> protecting them rather than having the user directly access them.
>
> Also, we had a convention of prefixing all data members with m_. Once
> you do that, it's cumbersome enough that you prefer to use accessor
> methods ;)
>
> 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.
>
> The code seems to be only for double, right? Is there a good reason to
> forbid float and other types?
>
> It's hard to make more comments, as I don't know this stuff and there
> 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.
>
> The functors idea seems to have been implemented right. Then again,
> not knowing about this stuff, I can't know if in this situation it's
> better to use functors (so static polymorphism, resolved at compile
> time) or virtual functions (so dynamic , runtime polymorphism). The
> functors approach allows to have more things known at compile-time and
> to make direct function calls.
>
> 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?
>
> 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.
> The prerequisite for this is to make very extensive unit tests, so one
> is sure not to introduce regressions.
>
> 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..?
>
> Benoit
>
> 2009/9/24 Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>:
>> Ok, finally a few remarks...
>>
>> 1) I think it might be of interest for users to change the cost function at
>> run-time. With the current design this is hardly possible since it requires
>> to instantiate a new LevenbergMarquardt class for each cost-function.
>> Pre-Instantiation of all possible optimizers is here a bit cumbersome. An
>> alternative would be to define a pure virtual cost function base class.
>> Actually, in our framework we have that even for optimizers since we also
>> like to switch between let's say Downhill Simplex (or Nelder Mead) and
>> LevenbergMarquardt. But that is really optional - though in our group we
>> utilize it a lot.
>>
>> 2) void abort() would be nice in the optimizer's interface. There exist
>> optimization procedures that take for longer than 5 minutes and here it
>> makes a lot of sense to be able to terminate early. How this might be done
>> a) Introduce a member: Status m_status;
>> b) Replace each 'return Running;' by 'return m_status;'
>> c) Replace each remaining 'return <status_type>' by 'm_status =
>> <status_type>; return m_status;'
>> d) Add 'void abort() { m_status = UserAsked; }
>>
>> 3) It seems, as if currently different functions are called for numerical
>> differentiation vs. optimal storage vs. analytical differentiation. I am
>> just wondering whether these special cases are not candidates for
>> templating. These are basically policies, right? Like 'use as much mem as
>> you want' vs. 'use minimal memory' and 'differentiate analytically' vs.
>> 'differentiate by forword diffs'.
>>
>> 4) Optional: A callback function would be cool. Basically, some way of
>> getting updates of intermediate optimization results. It oftentimes helps a
>> lot in the course of debugging/tuning specific optimization problems because
>> you can visualize the intermediate results.
>>
>> 5) Finally, I would not expose member variables directly and I am wondering
>> whether all of those functions are required in the public interface.
>>
>> In general I like the work - it's cool to have a non-linear optimization
>> based on Eigen. :) Thanks for your all the effort.
>>
>> Hauke
>>
>