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

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