| Re: [eigen] [PATCH] adding Reverse expression |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/eigen Archives
]
- To: eigen@xxxxxxxxxxxxxxxxxxx
- Subject: Re: [eigen] [PATCH] adding Reverse expression
- From: Gael Guennebaud <gael.guennebaud@xxxxxxxxx>
- Date: Tue, 3 Feb 2009 10:36:20 +0100
- Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:content-type :content-transfer-encoding; bh=GY6hh0rXSede0PSUcnsvBkbooZRbCGJe/pZP9dgtSyQ=; b=xAAJE+Ni2+oacj6Q+j10uwJyZAn5w4aGze2Bx76qpNFBsKyFBBGwXaPajHukaYCWwy eBU9mqNufMN+r2lGJD5f9TDiB/FD6X/HRwLr4ZsKeQXb/Pprfl4/REdF3FRNtiA29CYk XX0yqIk24x42cDLaR5DH+YzdbWInUblZ/jqsY=
- Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type:content-transfer-encoding; b=nVjiUtZDOql5Md+Xw2ShXrFZGj9XxcXmbbUZgU5hfnJYOa/IiVmrR24+IIvc/pVHWk QR4+M8FF2e3mvPUTE7Sqcshd3mcstcuNiPylE3BHt/oQGaNMsaIFdc/br08ctzpwmPti H6g6KZa77FEJBHmlPXMK8ccqQqjbGRHo+xtgk=
Hi,
some tips:
- to get the colwise/rowwise working correctly, I'd add a "int
Direction" template parameter to Reverse
- the name of the PartialRedux class is correct anymore and we should
find something else (DirectionalOperator, DirectionalWise, ???)
- the stride and nonZero overloads are not needed
- for the vectorization you need a ei_preverse() function, I guess I
can add it myself.
- it would be nice to add a unit test.
gael.
On Tue, Feb 3, 2009 at 1:41 AM, Ricard Marxer Piñón
<email@xxxxxxxxxxxxxxxx> wrote:
>
>
> On Tue, Feb 3, 2009 at 12:50 AM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
> wrote:
>>
>> Thanks for your patch; comments below:
>>
>> 2009/2/3 Ricard Marxer Piñón <email@xxxxxxxxxxxxxxxx>:
>> > Here is a patch to add the Reverse expression this is useful for
>> > flipping
>> > matrices, rows or columns.
>>
>> I can see that you declare a class Reverse, but don't actually define
>> it. You probably forgot to "svn add Reverse.h" ?
>
> ups yep. I had some trouble because I still had all the Toeplitz stuff and
> was trying to isolate the reverse stuff (aaah I wish I could get git svn to
> work)
> Anyway here is the full diff.
>
>>
>> Your PartialRedux::reverse() method returns a ExpressionType which has
>> 2 drawbacks:
>> 1) it requires ExpressionType to be writable, e.g. one can't use your
>> reverse() like this,
>> m3 = (m1+m2).rowwise().reverse();
>> 2) more importantly, your PartialRedux::reverse() is not lazy, so when one
>> does
>> v2 = v1.reverse();
>> the arrays are traversed twice: first a temporary vector is created by
>> reverse(), then this temporary vector is copied into v2.
>> This is of course inefficient. It is exactly in order to overcome
>> that, that we introduct expression templates ;)
>
> Yes the PartialRedux reverse is very drafty, I just wanted the API to be as
> I wanted it to be. I will try to get it right with a bit more work.
>
>>
>> Cheers,
>> Benoit
>>
>>
>
>
>
> --
> ricard
> http://www.ricardmarxer.com
> http://www.caligraft.com
>