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

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


cheers,
gael.