Re: [eigen] refactoring fork: some news

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

2009/12/18 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>
> Hi,
>
> some news from my fork: http://bitbucket.org/ggael/eigen2-et-refactoring/..

First let me say, thank you so much for all your work on that. At this
stage I'm not sure would have had the courage of doing such a deep
refactoring, but indeed it is very useful.

Just one question, have you measured the impact on compilation times,
since you are complexifying the inheritance tree?

> Let me first recall that the initial goal of this fork was to refactor our
> expression template tree so that is becomes easier to add and use new matrix
> types. In the same time I've added a true support for "Array" and started to
> clean the sparse module thanks the new possibilities offered by the new
> design. So in order to achieve this goal while minimizing code redundancy as
> much as possible, I've employed a couple of different mechanisms which are
> summarized on the following self explanatory scheme:
>
> http://eigen.tuxfamily.org/misc/eigen_new_design.xml ; (requires umbrello)

typo, it's .xmi

> http://eigen.tuxfamily.org/misc/eigen_new_design.png

Thanks a lot for the graph! Like Thomas, this helped me a lot
understand what you have done.

> Note that's not at all an UML diagram, I just tried to quickly picture all
> the major aspect of this new design on a single diagram. I hope it is clear
> enough, otherwise feel free to ask for precisions.
>
> In this fork I also moved all deprecated features to a new Eigen2Support
> module (i.e., lazy(), marked(),  .cwise(), etc.) In order to enable Eigen2
> compatibility you can either do:
>
> - #include <Eigen2Support> before any other Eigen header
> - #define EIGEN_EIGEN2_SUPPORT before any other Eigen header (in this case
> you don't have to #include Eigen2Support)

Great! Perhaps EIGEN_EIGEN2_ is redundant and EIGEN2_ is enough...

>
> Feel free to propose another solution.
>
> Currently the fork is good enough to pass all the unit tests, but it still
> requires a lot of work regarding cleaning, documentation, and the unit tests
> have to be extended to check for Array objects. But before going further I'd
> like to hear your opinion about this new design.

If compilation times havent dramatically increased, that's OK for me.
I haven't yet looked at the code in detail, sorry, job interviews, but
if you feel good about it, it's probably good...

> Another question: how to name the predefined typedef for Array ? The problem
> is to distinguish 1D and 2D array. So for I've use the following convention:
>
> Array33f => 2D 3x3 Array of float
> ArrayXXf => 2D dynamic Array of float
> Array3f => 1D Array of float of size 3
> ArrayXf => 1D dynamic Array of float
>
> but that's not consistent with Matrix's typedefs. Any idea?

I'd say that it's good enough, and one can hardly do better without
introducing a name difference between 1D and 2D arrays.

> Still about Array, let me recall that we agreed upon removing .cwise() in
> favor of a few cwiseXXX function for Matrix objects (e.g.,
> mat.cwiseProduct(mat), mat.abs()), plus a MatrixBase::array() function
> allowing to see a matrix as an Array, e.g.:
>
> (mat.array().abs()>10).select(A,B);
>
> instead of
>
> (mat.cwise().abs().cwise()>10).select(A,B);
>
> For the other way round, i.e., to see an Array as a Matrix I used the
> function asMatrix() because some expressions already defined a .matrix()
> function, that is not consistent.  So do you prefer to stick with
> .array()/.matrix() (in which case we have to rename the matrix() function of
> some expressions),

Yes, I think that's the right solution, because these matrix()/array()
methods will be what is used all the time by users, so they should
have a sweet name that doesn't require using the shift key. About the
other matrix() methods, I'm not even sure that it's a problem, but if
you think it is, they should be renamed anyway. Right now, I checked
in a few xpr classes such as CwiseUnaryOp, Transpose, Block, and they
don't have matrix() methods, so it doesn't seem like a big problem.

Cheers,
Benoit