Re: [eigen] [patch] LDLt decomposition with rank-deficient matrices

[ Thread Index | Date Index | More lists.tuxfamily.org/eigen Archives ]


Hi Manoj,

On Mon, Jun 28, 2010 at 6:49 PM, Manoj Rajagopalan <rmanoj@xxxxxxxxx> wrote:
>
>
>
> On Monday 28 June 2010 02:00:50 pm Ben Goodrich wrote:
>> Hi Benoit,
>>
>> On Mon, Jun 28, 2010 at 8:51 AM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
> wrote:
>> > Hi Ben,
>> >
>> > Let's get this sorted :) So far I didn't participate much in the
>> > discussion so I may be missing a lot of things.
>> >
>> > When it was question of making pivoting/nopivoting an option of the
>> > LDLT class, I thought we were talking about a template parameter. If
>> > fact, in your patch, it is a runtime option. In all other
>> > decompositions in Eigen, we have made this be separate classes, e.g.
>> > HouseholderQR vs ColPivHouseholderQR. Frankly I would like LDLT to do
>> > the same. Keep LDLT pivoting, and add a NoPivLDLT class. This will
>> > actually make your NoPivLDLT be more optimized (the biggest advantage
>> > will be smaller executable code). The only advantage of having that as
>> > runtime param is to share code if a user is using both flavors of
>> > LDLT. I believe that the most common case is that a program will use
>> > at most one flavor of LDLT.
>> >
>> > If you send a patch with this change, I promise I'll review it :)
>> >
>> > Benoit
>>
>> That is all fine with me; I don't have any strong reason to prefer a
>> runtime option. But I interpreted Gael's email on June 16 as asking
>> for a runtime option and then later he asked about the possibly of a
>> separate class. So, it seems we now have a consensus on the separate
>> class at least. :)
>>
>> Ben
>>
>
>
> Would it make sense to add an extra boolean template parameter to the LDLT
> template that defaults to 'true' with 'false' being the no-pivot case? Since
> partial specialization is allowed with class templates this should cause no
> problems with existing code that uses LDLT, AFAIK ...
>
> -- Manoj

That would work fine, but rather than booleans, Gael (and others, I
think) prefer

int options = Pivoting

where Pivoting is already mapped to some integer in
Eigen/src/Core/util/Constants.h .

Ben



Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/