Re: [eigen] patch for tridiagonal matrices in eigen

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

2008/12/15 Mauro Iazzi <mauro.iazzi@xxxxxxxxx>:
>I adapt them to the correct coding style (do you have guidelines written somewhere?).

No, but if you quickly read through the files MatrixBase.h and
Assign.h for example you get an idea of the style, then we're not too
maniac.

> I meant to define a new matrix class and specializing expressions,
> like product. It was very easy to do, so it's already very extensible,
> but I had to change the headers for that. Ideally I could just define
> without having to put new code in the library.

For example, as you saw, the logic choosing the product mode
(ei_product_mode) is in Product.h so you'd always have to change that.
The only way to do that from outside Eigen would be by partial
template specialization but that would basically require to know
beforehand which extra template parameters to do the specialization
on... not very convenient. That said I hear your point, there's always
room for improvement for better extensibility.

>> -const unsigned int Upper = UpperTriangularBit;
>> -const unsigned int StrictlyUpper = UpperTriangularBit | ZeroDiagBit;
>> -const unsigned int Lower = LowerTriangularBit;
>> -const unsigned int StrictlyLower = LowerTriangularBit | ZeroDiagBit;
>> +const unsigned int Upper = UpperTriangularBit | UpperHessenbergBit;
>> +const unsigned int StrictlyUpper = Upper | ZeroDiagBit;
>> +const unsigned int Lower = LowerTriangularBit | LowerHessenbergBit;
>> +const unsigned int StrictlyLower = Lower | ZeroDiagBit;
>>
>> here I don't really follow. These are parameters for part(). So unless
>> you want to add Hessenberg support in part (which would be a good
>> idea) no need to change that. If you want to add Hessenberg support,
>> you need to make the corresponding changes in Part.h.
>
> I saw that the product helper checked for diagonals using the flags,
> so I did the same for tridiagonals.

OK right, sorry. These are always convenient shortcuts. What needs to
be updated then is the comment saying that these are the parameters
for part(), actually they're not used only for part().

>> Here, the compiler will have to go through the whole logic of BOTH
>> cases, which means uselessly long compilation times. Yes the compiler
>> will always optimize the "if" itself (even in debug mode) so it's not
>> a runtime speed issue, nor a code size issue, it's really a
>> compilation-time issue, and we're extremely careful about that.
>
> understood. Maybe switching to MatrixType::SquareMatrixType also
> inside Tridiagonalization<> would allow to specialize it for the
> tridiagonal case. This would avoid storing and checking the flags and
> also the manual copy of the vectors, since we could use the ones that
> are already there (m_diagonalCoeffs, etc...).

I'd need to have a closer look at the code in order to be able to
comment on that.... (can't now)

>> And, the line
>>  if ( (Flags & Tridiagonal) == Tridiagonal ) {
>> can be written as just
>>  if(Flags & Tridiagonal) {
>
> I think that would incorrectly get Upper/Lower Hessenberg as
> Tridiagonals... I've seen (Flags & Diagonal) == Diagonal in other
> places for the same reason.

Oh right, nevermind. I didn't remember that diagonal (and tridiag)
were more than one bit...

we probably need a little comment when we do that,
 if ( (Flags & Tridiagonal) == Tridiagonal ) // Tridiagonal is two bits
and same thing for what we already do with diagonal...

Last note: it seems that you're already a kde contributor... so you'll
commit yourself.

Cheers,
Benoit

>
> cheers,
>
> mauro
>
> ---
>
>

---