Re: [eigen] Special Matrices: first get diagonal matrices done

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

I mostly agree.

On Mon, May 11, 2009 at 3:39 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
> First of all we could have a DiagonalMatrixBase::toMatrix() that
> returns a plain matrix (see at the end for naming discussion), so that
> the user who wants something that behaves like a matrix, can get a
> plain matrix if he wants to:
>
> cout << vec.asDiagonal().toMatrix() << endl;
>
> The argument for having instead a asDense() returning a MatrixBase, is
> to avoid the cost of evaluating the matrix, but then remember that
> this expression would have a very expensive coeff() method as it would
> have to do a if(row==col). So as soon as you're accessing a large part
> of the matrix, it's not good anyway (performance wise) to have an
> asDense expression here.

right, and I suggest toDenseMatrix() for the function name because we
also have that feature for sparse matrices (debug purpose mainly)

> I also think that there isn't much of a use case for doing directly
> "vec.asDiagonal()(i,j)" -- it's OK, i think, to force the user to
> replace that by "i==j?vec(i):0" making him realize where there is room
> for optimization.

that's clear.

> Is there anything I didn't think of?

the ability to write a generic template function taking any but only
Eigen's objects (dense xpr, diagonal, sparse, etc.) ?This would
require:
1) add a "meta" base class for all other base classes (MatrixBase,
Sparse...., Diagonal...)
2) have a set of common high level operations supported for and
between all types (matrix product, scalar product, add, sub, etc).

Currently I guess we can safely forget about that and still add it
later if it's really needed.

> Yes that means that one could no longer do:
> mat * mat2.marked<DiagonalBits>()

I think this should not be allowed.

> To summarize my plan:
> *no more changes to Diagonal (the ex-DiagonalCoeffs)
> *make DiagonalMatrixBase no longer inherit MatrixBase. It doesn't even
> need a coeff() method.
> *DiagonalMatrix still inherits DiagonalMatrixBase
> *DiagonalMatrixWrapper still inherits DiagonalMatrixBase
> *DiagonalMatrixBase is the class against which we implement the
> operator* catching diagonal products, and by the way we can also have
> operator= etc.
> *DiagonalMatrixBase::toMatrix() returns a plain Matrix.
>
> (the last method toMatrix(), i couldn't find a nicer name: eval()
> suggests it returns a DiagonalMatrix, toDense() doesn't make it clear
> it evaluates, ...)

again I suggest toDenseMatrix()...


Cheers,
Gael.