Re: [eigen] skipXxx / computeXxx parameters in Eigenvalues module

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

Note: this is very high priority items that I was considering doing if
you weren't. So your effort here is extremely welcome!!

Benoit

2010/5/30 Benoit Jacob <jacob.benoit.1@xxxxxxxxx>:
> 2010/5/30 Jitse Niesen <jitse@xxxxxxxxxxxxxxxxx>:
>> On Sun, 30 May 2010, Benoit Jacob wrote:
>>
>>> I would like us to align everywhere on runtime computeXxx parameters,
>>> not skipXxx parameters. In other words, 'true' means do compute. This
>>> is so we can avoid a double negation ("do not not compute"). Do you
>>> agree?
>>
>> Yes, that makes sense.
>>
>>> Then a separate question is what the default should be. Initially I
>>> thought default to true for ease of use. But actually, this makes it
>>> easy to write slow code, even unbearingly slow (for 10000x10 matrices,
>>> skipping the U computation is crucial...).
>>
>> Currently, one can only compute the Schur (or Hessenberg or eigen-)
>> decomposition of square matrices: given A, compute orthogonal U and a
>> triangular T such that A = U T U^*. It is not clear to me what a Schur
>> decomposition of a rectangular matrix is.
>
> Sorry, I was talking with JacobiSVD in mind. I didn't realize, but
> you're right, the whole Eigenvalues module is for square matrices. So
> I don't really know what should be the default, true or false. Perhaps
> true for ease of use.
>
>>
>>> So perhaps default to false and make sure that the corresponding assertion
>>> messages make it very clear what's going on ("You tried accessing this but
>>> you didn't ask for it to be computed in this decomposition").
>>
>> That raises another question. Some classes have an assert on an
>> m_isInitialized member variable to guard against the user requesting the
>> results before calling compute(). I guess all classes should have this.
>
> Yes, I agree.
>
>
>> But
>> in SelfAdjointEigenSolver, the member variable is only there if NDEBUG
>> is not defined:
>>
>>   class SelfAdjointEigenSolver : ...
>>   {
>>      ...
>>      #ifndef NDEBUG
>>      bool m_isInitialized;
>>      #endif
>>   };
>>
>> That seems quite nice - it saves a few bytes if NDEBUG is defined, when the
>> assert is not done anyway - but I haven't seen it elsewhere in Eigen. Are
>> there problems with it?
>
>
> Oh, i didn't know we were doing that.... who cares about saving a few
> bytes on a decomposition?? Decomposition objects are not typically
> something we store large arrays of. The sizeof doesn't matter,
> especially not when it's just a few bytes.
>
> Moreover, this makes the ABI of this class dependent on NDEBUG. While
> I never aimed to offer any ABI stability guarantee for decompositions,
> making the ABI depend on the build type seems a little over the board.
>
>>> Note that the Tridiagonalization and Hessenberg classes are special
>>> since the Q can rather be returned as a householder sequence,
>>> incurring no extra cost at the time of the decomposition.
>>
>> That's a good point, thanks. I hadn't realized that.
>>
>>> These classes should be rewritten as trivial adaptations of, respectively,
>>> UpperBidiagonalization and HouseholderQR.
>>
>> I don't understand this. If you know the QR decomposition, how does tat help
>> you to compute the Hessenberg decomposition?
>
> I didn't say that Hessenberg should internally call HouseholderQR. I
> meant that Hessenberg should be rewritten by just copying the code of
> HouseholderQR, and making some small adjustments. Indeed, it's almost
> the same thing, the only difference is that the role played by the
> main diagonal in HouseholderQR is now played by the first subdiagonal.
> If you look at the householderSequence class, you'll see a shift
> parameter that is exactly meant for that use. See how we do in class
> UpperBidiagonalization.
>
> Benoit
>
>
>>
>>
>> Jitse
>>
>>
>>
>