Re: [eigen] inconsistent cwise() support

On Tue, Nov 17, 2009 at 12:48 PM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

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