Re: [eigen] about JacobiSVD's options

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

ok so to clarify:

2009/11/22 Benoit Jacob <jacob.benoit.1@xxxxxxxxx>:
> - so: Options could be a bit-field with:
>  ComputeU = 1,
>  ComputeV = 2,
> and:
>  UseColPivHouseholderQR = 0
>  Square = 4,
>  UseHouseholderQR = 8
>  UseColPivHouseholderQR = 12

i meant UseFullPivHouseholderQR = 12

> JacobiSVD<MatrixType, ComputeV | AlreadySquare>

i meant Square, of course

also i've been thinking about these non-namespaced enums... for
potentially polluting names, we need to make sure we have a sensible
naming scheme. What would greatly help would be: all names that refer
to a shape, arranged as single-bit values starting at e.g. 64 (so no
conflict with small values that typically are used for specialized
uses). Like this:

Square = 64
SelfAdjoint = 128
Upper = 256
Lower = 512
ZeroDiag = 1024
UnitDiag = 2048

etc...

Benoit

>
> 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
>>
>