Re: [eigen] about JacobiSVD's options

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

2009/9/17 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
> On Wed, Sep 16, 2009 at 2:57 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
>> 2009/9/16 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>>> ok ok, but I still think it is a bit unfortunate to clutter the API
>>> for compilation time only.
>>>
>>> Actually, note that if the advanced user really want to speed up his
>>> compilation time, the best is to explicitly instantiate the few
>>> operations he need in a separate file (e.g., JacobiSVD<MatrixXd>), and
>>> then the compilation of JacobiSVD<MatrixXd> will take 0sec. I agree
>>> this is a little more work than just adding ",Square", but this
>>> solution works for all classes and global functions and it brings much
>>> more speed !
>>
>> But it doesn't address the issue of code size...
>
> well, that depends, because if someone use both non square and square
> then the code size increase (+4k per instantiation of JacobiSVD)

+4k in the case of more intensive usage isn't a big deal, compared to
+ dozens of k for the simplest use case.

(By the way my numbers were for non-stripped binary, stripping cuts
them in half)

There are a lot of users out there who will use only one SVD, and they
are more likely to care about code size than the one who use multiple
SVDs.

>
>> Does it really clutter the API to have an optional template parameter here?
>> It's optional, meaning that the user who doesn't care can just do
>>
>> JacobiSVD<MatrixXf> svd(m);
>>
>> Would you prefer this being split in a separate class,
>> SquareJacobiSVD? Or would you prefer it being a separate
>> constructor/compute() method, like this,
>>
>> JacobiSVD<MatrixXf> svd(m); // don't assume square, compile QR
>> JacobiSVD<MatrixXf> svd(m, IsSquare); // assume square. this ctor
>> takes a IsSquare_t argument
>>
>> Or would it be more acceptable if we had just Square and discarded the
>> AtLeastAsManyRowsAsCols which I agree isn't very useful?
>
> yes the AtLeastAsMany*As* are definitely useless and they can only
> increase the code size.

OK.

> One problem is that the SkipU/SkipV options are mixed with the
> Square/NonSquare one, and so if someone use SkipU then he loses the
> automatic computation of Square/NonSquare for fixed sizes,

No, I disagree, unless you saw a bug that I don't see in my code?

> and so he
> would have to duplicate that piece of code... So if you really really
> want to keep Square, then it should be a separate template parameter.

Again, I thought that I had implemented correctly the bit-field idea,
you seem to have seen a bug?

> oh! I've just got an idea: what about making the Square/NonSquare
> option a true feature ? The idea is to template JacobiSVD by a
> preconditioner type which would allow the user to choose between:
>
> - Identity (for square matrix)
> - FullPivotingHouseholderQR
> - ColPivotingHouseholderQR
> - etc.
>
> Currently there is no big advantage of using ColPivotingHouseholderQR,
> but once we'll have a blocked version of it, performance wise, it will
> be very relevant to offer that feature. What do you think ?

Indeed, I was thinking about letting the user choose the QR preconditioner.
I was thinking of adding it as an additional template template parameter.
But your idea (of letting it replace the Square/... option) sounds
good, I need to think but that seems to work, thanks!

Benoit