Re: [eigen] proposal for "clean" output arguments

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

2009/8/19 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
> On Wed, Aug 19, 2009 at 3:07 AM, Keir Mierle<mierle@xxxxxxxxx> wrote:
>> On Tue, Aug 18, 2009 at 7:31 AM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx>
>> wrote:
>>>
>>> Hi all,
>>>
>>> here is a proposal to deal with functions having output (or inout)
>>> arguments. Currently the situation is quite a mess:
>>>
>>> 1 - some take non const references like
>>> TriangularView::solveInPlace(MatrixBase<T>&)
>>> 2 - some take pointers like LU::solve(const MatrixBase<T1>& B,
>>> MatrixBase<T2>* X)
>>> 3 - some take const references like MatrixBase<T1>::swap(const
>>> MatrixBase<T2>&)
>>>
>>> The main advantage of the solution 3 is that allows to use temporary
>>> proxies, e.g.:
>>>
>>> m.col(i).swap(m.col(j));
>>>
>>> The main advantage of solution 2 is that it makes it clear what is an
>>> output:
>>>
>>> m.lu().solve(b, &x);
>>>
>>> The respective drawbacks of each method are pretty obvious I won't
>>> enumerate them one more time...
>>>
>>> So what I propose is to add a trivial Output<T> class mimicking a
>>> reference T& that we'll be passed by value. Ok to make thing crystal
>>> clear here is such a Output class:
>>>
>>> template<typename T> class Output
>>> {
>>>  public:
>>>    Output(T& obj) : m_object(obj) { }
>>>    operator T&() { return m_object; }
>>>  protected:
>>>    T& m_object;
>>> };
>>>
>>> then we add a output() member function to AnyMatrixBase<> :
>>>
>>> Output<Derived> output() { return derived(); }
>>>
>>> then the function LU::solve(const MatrixBase<T1>& B, MatrixBase<T2>*
>>> X) can be rewritten:
>>>
>>> LU::solve(const MatrixBase<T1>& B, Output<T2> _x) {
>>>  T2& x(_x);
>>>  // use x
>>> }
>>>
>>> and the user sill call it like this:
>>>
>>> mat.lu().solve(b, xs.col(2).output());
>>>
>>> For in-out argument we can do the same with a InOut<T> class, and a
>>> AnyMatrixBase::inout() function.
>>>
>>> Unless I missed something, I think this solution has all the
>>> advantages that someone can expect:
>>> - it is more C++ than pointers,
>>> - it respects constness (unlike const references)
>>> - it allows to use temporary proxies returned by a function (unlike
>>> references and pointers)
>>> - it make it crystal clear what is an output and an in-out arguments
>>> when someone read a piece of code (unlike all other solutions)
>>>
>>> The only limitation I can see is how to extend this concept to scalar
>>> type arguments because we cannot have:
>>>
>>> double x;
>>> x.output()
>>>
>>> Note that if we don't make the ctor of Output explicit, then the
>>> following we'll work:
>>>
>>> void foo(Output<float> _x);
>>>
>>> float x;
>>> foo(x);
>>>
>>> If we want to enforce to have "output" next to x, one possibility is
>>> to add a global function output(T&) and make the ctor of Output
>>> explicit:
>>>
>>> float x;
>>> foo(output(x));
>>>
>>> Note that such a global function will only work on declared objects,
>>> and not on temporary proxies, that is why we really have to also have
>>> the output() function as a member of AnyMatrixBase...
>>>
>>> Of course, another drawback is more API changes...
>>>
>>> What do you think ?
>>
>> I'm mildly opposed to this because I don't find the .output() or .inout()
>> notation clearer. Specifically, I suspect that if you put the following to
>> C++ programmers who know math but not eigen:
>> Vector3d x, b;
>> Matrix3d A;
>> b = ...  // fill in b
>> A = ... // fill in A
>> A.lu().solve(b, x.output());
>> They would not guess what output() means. Does that mean output to the
>> screen?
>> I suggest having a .reference() or .ref() method instead of output() or
>> inout(); that makes it clear that you're passing a reference. I think
>> there's little value in specifying the difference between out and inout; it
>> should be clear from the caller's code.
>> If Output<T> has an implicit constructor taking a pointer to T, then there
>> is no need to  break the API. This way, & notation is supported for simple
>> matrix types, and there is .ref() to get a mutable reference to a temporary.
>> A.lu().solve(b, &x);
>> A.lu().solve(b, x.block<3,10>(0,0).reference());
>> Keir
>
> I think you are right in having only a .ref() method. However,  while
> adding a ctor Reference(T*) seems to be a nice trick to easily keep
> backward compatibility, this won't work in our case because all our
> functions are templated, and the compiler cannot instantiate a
> template function if one of the argument requires a type conversion
> where the type of the argument depends on a template parameter (it can
> only do such a conversion via inheritance).
>
> So, e.g., assuming we have the two following implicit ctors:
>
> Reference(T&)
> Reference(T*)
>
> and this function foo:
>
> template<typename T> foo(Reference<T> a, Reference<int> b);
>
> MatrixXi x;
> int i;
>
> then the following will work:
>
> foo(a.ref(), i);
> foo(a.ref(), &i);
> foo(a.ref(), ref(i));
>
> but not that:
>
> foo(a, ref(i))
> foo(&a, ref(i))
>
>
> Likewise, if foo is declared like this:
>
> template<typename T> foo(Reference<T> a, Reference<typename T::Scalar> b);
>
> then you must create a Reference object yourself for the second argument:
>
> foo(x.ref(), ref(i)); // works
>
> foo(x.ref(), i);   // does not work
> foo(x.ref(), &i); // does not work
>
>
> That also means to avoid any unexpected behavior we should make the
> Reference(T&) ctor explicit.

I just thought about something. For swap(), i think it would be really
too cumbersome to force the user to write

x.swap(y.ref());

More generally: it's good to force the user to write ref() when that
makes clear which argument is the output; but not when there's no risk
of confusion because e.g. everything is an output (this is the case
with swap). also in swap it would be good to preserve the symmetry
between x and y.

I know this makes the game even more complicated :( one can always
treat swap separately so it doesn't hold back the rest.

actually here's the situation with swap. either y is a plain matrix,
in which case we can pass it by non-constant reference, or y is an
expression, in which case we can pass it by value. all what's needed
is to find a way to determine that at the moment of the function call.
if that's not possible then we can at least do metaprogramming inside
swap to enforce constness.

Benoit