Re: [eigen] refactoring fork: some news

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

2010/1/4 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.)

Great! As long as you do the work ;)

Here's an idea for the meeting in February: would you like to do us a
presentation on the changes that your refactoring brings, especially
on the class hierarchy, walking us through some code for each of the
new/affected classes? I know i'm busy at the moment and not very
likely to study these changes in detail soon. Since you have this nice
diagram already prepared, could you build a presentation (30 min
roughly?) around it?

Benoit