Re: [eigen] inconsistent cwise() support

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

Hi,

I agree 100% with your analysis.

About the fact that .array() will have to be written on both sides of
binary operations, one must keep in mind that this will be
considerably alleviated by the availability of the new Array class as
a storage type. Most of the code that does a lot of .cwise() today
will actually be simplified when Array will be available, so not too
much code will have to do .array() with binary ops.

About the big API break, here's a proposal: let's create a new module
Eigen2Support that #includes<Eigen/Eigen> and adds more stuff to make
it as compatible as possible. So you could just put the Cwise class,
more or less unchanged, there. There are a lot more API breaks that
could be softened by having such a module.

Benoit

2009/11/17 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>
> I also agree with that proposal, but I'm thinking that once we'll have a
> true array support, all this array/cwise stuff is going to be messy. Here
> I'll assume that a true Array class supporting the standard global math
> function (e.g., std::sin) is really needed. If that's not the case, then we
> don't implement it => problem solved. So once we'll have this Array class, I
> guess we'll also want an easy way to view a Matrix as an Array, so one will
> be able to write:
>
> MatrixXf m;
> abs(m.array());
>
> that is redundant with the current:
>
> m.cwise().abs();
>
> Since we are currently in favor of member functions rather than global
> functions, we might also want to add, e.g.,  an abs() method to Array
> yielding a third option for the user:
>
> m.array().abs();
>
> For consistency reason, perhaps we would also support the cwise() API for
> Array. After all it would be weird that this cwise() API works for Matrix
> only. So now we would also have three options for Array objects:
>
> ArrayXf a;
> abs(a);
> a.abs();
> a.cwise().abs();
>
> So the addition of such an Array class is perhaps the occasion to reconsider
> coefficient wise operations. In order to make a decision, it is also
> interesting to have a look at some binary operations:
>
> MatrixXf m1, m2;
> ArrayXf a1, a2;
>
> m1.cwise() + m2;    // current API
> m1.array() + m2.array(); // more verbose, but symmetric
>
> a1 + a2;
> a1.cwise() + a2;   // very stupid
>
> m1.cwise().max(m2); // current API
> max(m1.array(), m2.array()); // more verbose, but symmetric
> m1.array().max(m2.array());  // more verbose, not really symmetric
>
> max(a1,a2);
> a1.max(a2);  // not really symmetric
> a1.cwise().max(a2);   // very stupid
>
> I think it is also important to acknowledge that the current cwise() API is
> odd enough to make it hard to learn for new comers. They often have some
> difficulties to determine where .cwise() is needed (e.g., m1.cwise() *
> m2.cwise()), and how it propagates (though it does not, of course). For
> instance, if you look at "m1.cwise() * m2" and you know, or did not
> understand the prefix concept of .cwise(), then it seems that .cwise
> propagate to m2.
>
> Ok I think you got me now. When we designed this cwise() API, I liked it a
> lot because it was fun: it allows us to overload a couple of operators
> (*,+,-,<,>,!=, etc.) but now, it looks more odd than fun, and this is
> something that annoys me in Eigen.
>
> Said that, I still don't have an ideal solution to propose, but what about
> removing the .cwise() prefix in favor of only the .array() style that is
> illustrated above ? To help you make a decision here are the four main
> consequences of such a change with their own pro and cons:
>
> 1 - It makes the API simpler once Array is implemented (+1)
>
> 2 - For binary operations you need to put Array on both sides, so it is, at
> a first glance, a bit more verbose (-0.5). On the other hand it is perhaps
> less cumbersome (+0.5).
>
> 3 - When you do: (m1.array() * m2.array()) you get an Array expression, not
> a Matrix. So you might need to convert it back explicitly to a Matrix
> (-0.5):
>
> (m1.array() * m2.array()).matrix() * m3;
>
> On the other hand this avoid the need to repeat .cwise() all the time when
> all your operations are coefficient wise, e.g.:
>
> (m1.array() * m2.array()).abs().pow(alpha);
>
> // or:
>
> pow(abs(m1.array() * m2.array()), alpha);
>
> Since it seems to me that you very rarely have to mix pure Array operations
> with pure Matrix ones in a single expression, this seems to be a nice
> advantage (+1).
>
> 4 - Of course the last consequence is a significant API break, and here I
> only see a strong drawback, I'm thinking hard, but I cannot find any
> advantage for this consequence (-1).
>
> For the rest it is pretty much the same than for the current cwise()
> solution (operator overloading, for unary ops it is just a text replacement,
> etc.).
>
> At the end the sum of the scores is positive, +0.5, so that's a proof this
> is a good proposal ;) Seriously, it seems to me that this proposal has some
> real benefits for practical uses, but of course the major blocking point is
> the API break.
>
>
>
> Finally, we can also discuss the choice to have both global functions and
> member methods for Array. Here global functions are really useful, that's
> why I'm in favor to provide both.
>
>
> Cheers,
>
> Gael, who likes to put on the table tough API discussions, over and over
> again ;)
>
>
> On Mon, Nov 16, 2009 at 9:40 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
> wrote:
>>
>> 2009/11/16 Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>:
>> > On Mon, Nov 16, 2009 at 7:22 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
>> > wrote:
>> >>
>> >> It seems like what you want is Gael's proposal for "true array
>> >> support".
>> >
>> > Not really, since I am really required to work on matrices - actually I
>> > was
>> > working on spectral clustering involving eigenvalue decompositions (so
>> > real
>> > linear algebra stuff).
>> >
>> >>
>> >> vector - scalar is not a standard operation; it doesn't have a
>> >> geometric meaning. Actually matrix - scalar is sometimes used as
>> >> matrix - scalar*Identity, so that's another reason not to let it mean
>> >> coeff-wise substraction.
>> >
>> > I totally agree, I was just thinking about consistingly supporting (I do
>> > even volunteer to implement it)
>> >
>> > .cwise() += | -= | /= | *=
>>
>> oh, i see.
>>
>> Yes, I agree with your proposal, it would be cool if you implement it.
>> It's true that there's no reason not to let .cwise syntax always work.
>>
>> Benoit
>>
>>
>> >
>> > and not in general
>> >
>> > MatrixBase::operator += etc.
>> >
>> > I just think that some problems (formulated in an algebraic framework)
>> > do
>> > sometimes need individual rescaling and sometimes even shifting when you
>> > need to work e.g. on zero-mean input data.
>> >
>> > - Hauke
>> >
>>
>>
>
>