Re: [eigen] refactoring fork: some news

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

On Sat, Dec 19, 2009 at 12:52 AM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
> 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.

yes, it was more work than I though. I'm always much too optimistic :)

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

That was also one my main concern.... fortunately the overhead seems
to be less than 1%.

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

oh thanks.

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

good because it was already EIGEN2_SUPPORT :)

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

ok, done.


Finally, let me add that I've recently cleaned it a bit and synced it
with the devel branch. It still requires an effort regarding the
documentation and the unit tests but I think we could consider to
merge it asap in order to reduce further merging efforts (API changes,
etc.)

gael.