Re: [eigen] about JacobiSVD's options

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

ok so i'm finally getting back to the SVD module and starting by
applying the outcome of this discussion.

So: JacobiSVD<MatrixType, Options> where Options was the subject of this debate.

- we already to remove the AtLeastAsManyRowsAsCols, and to add a way
to control the QR preconditioner.
- after all, a template template param for the preconditioner is not
scalable enough: what if some future QR class takes a 2nd template
param, etc.
- so: Options could be a bit-field with:
 ComputeU = 1,
 ComputeV = 2,
and:
 UseColPivHouseholderQR = 0
 Square = 4,
 UseHouseholderQR = 8
 UseColPivHouseholderQR = 12

As always with bit-fields, we must let the default be 0, so that
people who want to pass non-default options don't need to OR that with
the default.

So here the default is: use ColPivQR, and don't compute U or V. Does
that sound sane? The rationale for defaulting to not computing U/V is
that it is much more natural to ask for what you want (and here if you
forget to you get a very explicit error message) than to ask for what
you don't want.

If you know the matrix is square and you want V and not U, you do:

JacobiSVD<MatrixType, ComputeV | AlreadySquare>

Benoit
PS... we are still not namespacing enum values... as long as we can
avoid to, that's good, i just hope we can go on this way forever. Here
I made sure that 'Square' is a power-of-2 so it's easy to reuse in
other bit-fields.


2009/9/17 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
>