Re: [eigen] resizing in ReturnByValue assignment

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

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).

> I did run the regression tests with and without the initial resize. The same
> tests passed and failed in both cases.
> This would seem to support my theory that most implementations of
> ReturnByValue<Derived>::evalTo resize the destination matrix.

I just checked, and some ReturnByValue implementations indeed do call
resize(). I would call that the bug!

> Can anyone come up with a concrete example of why resizing is necessary
> *before* calling evalTo?

What is necessary is to have the ReturnByValue class provide
rows()/cols() methods, to allow to construct temporary matrices of the
right size when it's nested into bigger expressions,

That is your real problem. It means that ReturnByValue is not usable
in your situation where (quoting you) "the size of the output depend
on the TYPE of output requested.".

You need to pass enough information to your ReturnByValue-returning
function, that it knows the size of the output.

>
>
> -- Mark
>
>
> Benoit Jacob wrote:
>>
>> 2010/3/8 Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>:
>>
>>>
>>> Resize should be almost like a nullary op when the destination object
>>> already has correct size - if it is not, this is a bug. The advantage
>>> of having the assignment operator doing the resizing is that you
>>> cannot forget about it when implementing ReturnByValue since you
>>> simply don't need to take care of it.
>>>
>
> I admit that the FFT ReturnByValue has a bug.  The problem is the bug
> compiles correctly and gives the right answer.  This is the hardest kind of
> bug to track down.

Hehe.

> By removing that initial resize, the path to using ReturnByValue is less
> error-prone.
>
>>> Regarding the confusion about rows()/cols() you have to think of
>>> ReturnByValue as your actual result and thus this object should also
>>> be reflecting your result's properties as e.g. rows() and cols().
>>>
>>
>> exactly... think for example of the case where you are using your
>> returnByValue object in some expression that wants to evaluate it as a
>> temporary (as is automatic when nesting ReturnByValue into a bigger
>> expression)... then it needs to know the dimensions of the temporary
>> matrix to create!
>>
>
> Won't this created matrix then be the argument to evalTo?  It can assume
> responsibility for resizing.

Again, this is not the real issue at hand. This part is just cosmetic.
Given that rows()/cols() is available in ReturnByValue, it's just a
bit simpler (it centralizes 1 line of code once and for all in the
ReturnByValue base class).

If I understand you correctly, your real problem is that you would
like not to have to implement rows()/cols() in ReturnByValue.

Benoit

>
>> Benoit
>>
>>
>>>
>>> Hope that helps...
>>>
>>> - Hauke
>>>
>>> On Mon, Mar 8, 2010 at 2:33 PM, Mark Borgerding <mark@xxxxxxxxxxxxxx>
>>> wrote:
>>>
>>>>
>>>> First, the praise: I really like the ReturnByValue paradigm.  It is
>>>> syntactically cleaner than using the destination-as-an-argument. It
>>>> bends
>>>> the rules of c++ somewhat and allows one to overload/specialize based on
>>>> return type.  Very cool.
>>>>
>>>> Next, my confusion:
>>>> Eigen::FFT fwd,inv now uses ReturnByValue but it elicits an unnecessary
>>>> resize in some conditions (eliminating the advantage of the
>>>> ReturnByValue
>>>> proxy object). This is my bug -- I thought the rows() and cols()
>>>> referred to
>>>> the source matrix. They refer to the destination.   I can fix this by
>>>> duplicating the logic required to decide the output size from the FFT
>>>> fwd
>>>> and inv functions, but I'm lazy and I don't like doing work I don't need
>>>> to
>>>> do.
>>>> Why does DenseStorageBase::operator=( ReturnByValue ) call resize()
>>>> first?
>>>>  (defined in Eigen/src/Core/DenseStorageBase.h around line 300)
>>>> Why not just require the ReturnByValue subclass evalTo method resize the
>>>> destination as needed?
>>>>
>>>> Removing the resize also simplifies the implementation slightly by
>>>> eliminating the need for rows() and cols() to be defined in the
>>>> ReturnByValue subclass.
>>>>
>>>> Am I missing something?
>>>>
>>>> -- Mark Borgerding
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
>
>