Re: [eigen] General design issue

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

On Fri, Nov 13, 2009 at 8:58 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:

Hi,

Wow, your mail took me a lot of effort to understand, I think it's
mostly ok now...

sorry for that, I should have been more explicit.
 

First of all, I agree with your summary of current limitations. Yes, I
agree we must take care of that before beta1.

Let me rephrase some stuff in my own baby words to see if i got it
right. The 2 basic problems you want to address are:
* code sharing between dense and sparse xpr
* in xpr trees, only the leaves should be affected by what the leaves
actually are.

The "naive" approach to that would probably be to let e.g. Transpose
and SparseTranspose inherit a common base AnyTranspose, making it a
curious base so AnyTranspose<Derived>... then .transpose() methods
should return objects of AnyTranspose<Derived> which is ugly and may
(out of empirical experience) give compiler warnings about strict
aliasing when casting the returned object to the derived type.

So your solution, if I understand well, consists instead in letting
Transpose itself be a template in one more integer parameter, which
you call StorageType, which could have special values Dense, Sparse,
Band...

At this point, I think I have understood how you solve the "the rest
of the tree doesn't completely change if the leaves are different"
problem.

But can you enlighten me as to how you solve the "code sharing"
problem? If Transpose<Dense> and Transpose<Sparse> can share code,
where do you put that? In a common base class TransposeBase? So that
problem and solution is actually completely orthogonal to the above?

What I suggest is a bit different. Let's pick an example:

template<MatrixType> Transpose : public TransposeImpl<MatrixType, ei_traits<MatrixType>::StorageType>
{
 int rows();
 int cols();
 MatrixType::Nested m_matrix;
 // etc.
}

then TranposeImpl<MatrixType,Dense> would inherit MatrixBase<Transpose<> > and implements the coeff() methods while TransposeImpl<MatrixType,Sparse> would inherit SparseMatrixBase<> and implement the InnerIterators mechanism.

Again I'm not sure that's the ultimate solution, but at least it seems to improve the situation while being harmless.

gael.
 

Benoit

2009/11/13 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:

> Hi,
>
> I'm more and more convinced that our current design of our _expression_
> template mechanism is still not very good and some improvements are required
> to make it more scalable to new matrix types. For instance, for sparse
> matrices I had to create clones of most of the _expression_ classes we have in
> the Core module. E.g., there are SparseCwiseBinaryOp, SparseCwise,
> SparseTranspose, etc. Even though, e.g., CwiseBinaryOp and
> SparseCwiseBinaryOp have only very little in common, this till represent a
> non negligeable bit of code duplication. Especially the Cwise proxy class
> are essentially the same.
>
> Even though we would only correctly support dense and sparse matrices, there
> is still a major limitation: one cannot write the type of a generic
> _expression_ dealing with either dense matrices or sparse matrices. For
> instance see what I had to do for the AutoDiff module: instead of simply
> writing CwiseBinaryOp<ei_scalar_sum_op<Scalar>,A,B>, I introduced a
> MakeCwiseBinaryOp<> little struct in Core/ExpressionMaker.h returning the
> correct type of a cwise binary op accoring to the kind of
> matrices/expressions we are dealing with. Such and helper class has to be
> written for every _expression_ types and specialized for every kind of
> matrices.
>
> Likewise one cannot write generic _expression_ analyser only because the name
> of the template class representing, e.g., a transpose operation, depends on
> the kind of matrices!
>
> Of course things are getting wors as we want to support new kind of matrix
> storages. You might argue that matrices with special storage are only useful
> for solving, but you always need matrix products involving scalar multiples,
> transpose, conjutage, etc. So it seems to me that finaly all kind of
> matrices need a pretty much complete support for _expression_ templates.
>
> Well, all in all, I'd like to investigate some solutions to make this whole
> mess a little less messy, and yes I think this is something very important
> to do for 3.0 beta1. Well, unless someone show me the current design is
> already perfect....
>
> OK, so the general idea is to try to decouple the semantic of an _expression_
> from its implementation. E.g.: the type of 2*M should always be
> CwiseUnaryOp<ei_scalar_multiple<Scalar>, typeof(M)>  even though it is
> evaluated differently according to the storage of M.
>
> To this end, I think we could introduce a "StorageType" integer to the
> matrix traits. We could also use the Flags, but adding a new attribute for
> that seems to be easier to use and more scalable.
>
> Then, each _expression_ could inherit an implementation class where one of its
> template parameter would be the StorageType. This way one could add new
> storage type and only focuses on its implementation specificities. The
> implementation of all basics operators (all unary op, +, -, the entire
> .cwise() prefix, etc.) could be shared for all matrix types.
>
> Of course I'm still unsure about many of the details and I probably overseen
> a lot of difficulties, but it seems to be a saner design for the future.
> Discussions are open ;)
>
> Cheers,
> Gael.
>
>