Re: [eigen] resizing in ReturnByValue assignment

[ Thread Index | Date Index | More lists.tuxfamily.org/eigen Archives ]




On Mon, Mar 8, 2010 at 5:27 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
2010/3/8 Mark Borgerding <mark@xxxxxxxxxxxxxx>:
> On 03/08/2010 09:56 AM, Benoit Jacob wrote:
>>
>> 2010/3/8 Mark Borgerding<mark@xxxxxxxxxxxxxx>:
>>
>>>
>>> My concern is that there is no value added by the initial resize.  At
>>> best
>>> it is a NOOP.  At worst, it is a silent performance killer when resizes
>>> don't match exactly.
>>>
>>> In the case of Inverse FFTs, the size of the output depend on the TYPE of
>>> output requested. This is NOT available during the construction of the
>>> proxy
>>> object.  It is not available until the evalTo is called.
>>>
>>
>> Strange. We didn't design ReturnByValue for this use case. So your
>> problem is deeper: you can't, then, implement rows()/cols() in the
>> ReturnByValue-derived class. This means that, calling foo() your
>> function returning a ReturnByValue object, you can't support general
>> arithmetic expressions like
>>
>>     some_matrix + foo()
>>
>> as that would require foo() to evaluate into a temporary matrix, but
>> it can't know its size (or that would be far too complex logic to
>> determine in general from the _expression_ it is being nested into).
>>
>
> Now I see the need for the rows,cols in nested expressions.  This need is
> not evident in the simple case
>  some_matrix = foo();
> but
>  some_matrix = foo()*42;
> is nested and requires more knowledge.
>
>
> Actually, the limitation goes well beyond not knowing the size. The
> ReturnByValue needs to know the type of temporary to be created first (by
> ReturnType typedef).
>
> This knowledge can be provided by casting to a temporary (see example code
> below), but that is not a perfect solution either, since it requires actual
> allocation of a temporary, rather than a chainable placeholder.
>
>    // pre-example code
>    VectorXf real1,real2;
>    VectorXcf cpx1;
>    real1.setRandom(len);
>    FFT<float> fft;
>    fft.SetFlag( fft.HalfSpectrum );
>    fft.fwd(cpx1,real1);
>
>    // EXAMPLE CODE
>    fft.inv(real2,cpx1); // #1 original style calling convention, destination
> is first arg
>    real2 = fft.inv(cpx1); // #2 this succeeds, but has an extra resize in
> DenseStorageBase::operator=
>    real2 = fft.inv(cpx1)*42; // #3 this fails to compile -- cannot infer
> result type
>    real2 = VectorXf(fft.inv(cpx1))*42; // #4 this succeeds, but has extra
> resize from #2 plus a heavyweight temporary object
>
> The fact that #3 does not work and #4 is not efficient does not detract from
> how much prettier #2 is compared with #1.  I very much like the style of
> ReturnByValue. I refuse to give up on it yet for Eigen::FFT.

Why don't you template your inv() method in the destination scalar type? Like:

 real2 = fft.inv<double>(cpx1);
vs.
 cpx2 = fft.inv<complex<double> >(cpx1);

This would be in line with our general design decision of doing almost
no implicit casting.

Or another option: since these are 2 different notions why not 2
different methods?
 real2 = fft.realInverse(cpx1);
 cplx2 = fft.cplxInverse(real1);


I second that proposal. As a bonus it also improves the readability => no need to know precisely the types of dest to understand what's going on...

gael
 
of course you could still keep inv() for the case where the
destination type is identical to the scalar type...

>
> Option 1.
> Is there a proxy matrix type that would allow one to explicitly name the
> type of matrix expected, but defer its actual creation until later?

no... that's complicated!

>
> Option 2.
> Alternately, I propose a companion, simpler, ReturnByValue class that
> retains most of the syntactic benefits of the ReturnByValue
> , but cannot be nested in an _expression_ (unless explicitly casted).  Perhaps
> a specialization of ReturnByValue would provide the simpler proxy I need.

But it would be confusing for the user that certain ReturnByValue
pseudo expressions can be nested, and some can't.

Benoit

>
> -- Mark
>
>
>





Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/