This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen

Bug 111

Summary: Public API to handle function arguments
Product: Eigen Reporter: Gael Guennebaud <gael.guennebaud>
Component: Core - expression templatesAssignee: Nobody <eigen.nobody>
Status: DECISIONNEEDED ---    
Severity: Unknown CC: chtz, gael.guennebaud, jacob.benoit.1
Priority: ---    
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on: 481    
Bug Blocks: 138, 1608    

Description Gael Guennebaud 2010-11-08 13:20:17 UTC
This comes from the end of the thread: "new tutorial on writing functions taking Eigen types as paramters".

The goal is to provide a simple and stable API to deal with generic function arguments such as:

template<Derived> void foo(const MatrixBase<Derived>& mat)
{
}

where Derived can be any Eigen expression. This will become even more important once bug 99 fixed.

If mat is used only once in the function, then it's fine, the user has nothing to do.

If mat is used N times, then the expression will be evaluated N times that is not good.

Even worse, if the function foo directly access to the coeff of the matrix mat, e.g., for each i,j do something with mat(i,j)..., then compilation might fail if the expression has to be evaluated first (e.g., if it contains products, or result of a solve operation, etc.).

In the previously mentioned ML thread we proposed something like:

template<int N> struct NestedForMultipleAccess {
  typedef typename ei_nested<Derived, N> Type;
};

typename XprType::template NestedForMultipleAccess<N>::Type myvar(myarg);

or just:

typename XprType::template Nested<>::Type        myvar(myarg);
typename XprType::template Nested<1>::Type       myvar(myarg);
typename XprType::template Nested<2>::Type       myvar(myarg);
typename XprType::template Nested<Dynamic>::Type myvar(myarg);

and maybe some shortcut to avoid the need of the template keyword most of the time:

typedef typename ei_nested<Derived,1>::type       Nested;
typedef typename ei_nested<Derived,2>::type       NestedForTwoAccess;
typedef typename ei_nested<Derived,Dynamic>::type NestedForMultipleAccess;

alternative names: NestedOnce, NestedTwice, NestedManyTimes ???


All these names are based on the word "Nested" which is actually not very good here because the 1) the expression is not really nested here, and 2) after bug 99 implemented, this "Nested" mechanism won't be based on our internal::nested anymore.

So name more focused on the task of handling function arguments would be better suited I guess.
Comment 1 Benoit Jacob 2010-11-08 18:20:50 UTC
Here's what I wrote in bug 99 comment 16:

> It's a really far-reaching change: did you consider that it completely changes
> internal::nested, as this should no longer do any cost estimation at all.
> Basically, nested would only take care to nest xprs by value and plain objects
> by reference; and then the cost estimation would go to a separate helper
> should_evaluate_temporary computing a boolean value, used mostly in the
> evaluator<> for certain xprs like products.

So this is my opinion on what you say in comment 0: internal::nested becomes almost trivial ("return T if abstract xpr or T& if plain object"), as nothing happens at nesting time anymore, the complexity moves to the evaluator internals.

For this reason, as I say in bug 99 comment 18, I believe that the current way of writing functions taking Eigen arguments will still work,

template<typename Derived>
void myfunction(const DenseBase<Derived> & xpr)
{
  typename Derived::Nested x(xpr);
  // do some work with x ...
}

however this Nested stuff will just become unnecessary and people will be able to just use xpr directly once we have made the evaluator changes, right?

Also notice that even if it turns out that we don't have yet a good API-stable way of letting users write functions taking xprs as arguments, rather than blocking the 3.0 release on that, we could say that's currently not supported i.e. our xpr templates are still an internal thing and people should for now pass plain objects as arguments if they want something API-stable. I'd prefer that over delaying the 3.0 release or jeopardizing it with a last-minute huge deep change (evaluator system).
Comment 2 Gael Guennebaud 2010-11-08 18:38:43 UTC
hm... let's pick a stupid but more concrete example:

template<typename Derived> void foo(const DenseBase<Derived>& xpr)
{
  typename Derived::Scalar min, max;
  min = max = xpr.coeff(0,0);
  for(int j=0;j<xpr.cols();++j)
    for(int i=0;i<xpr.cols();++i) {
      min = std::min(min,xpr.coeff(i,j));
      max = std::max(max,xpr.coeff(i,j));
    }
  // ...
  typename Derived::PlainObject X = (xpr.array() - min)/(max-min);
}


Let's call:

ArrayXf A, B;
foo(sin(A)+cos(B));

The expression sin(A)+cos(B) will be evaluated 3 times ! Even if I copy xpr into a Xpr::Nested.

Even worse:

MatrixXf A,B;
foo(A*B);

=> this won't compile!

whence our paste discussion about adding an official NestedForMultipleAccess<> like stuff, and document it. But in 3.1 these official Nested* stuff won't make much sense since they be based on internal::nesting at all. and consequently it would be better to find a better yet consistent naming scheme for this mechanism ...

Is it clearer? or maybe it is me who is missing something??
Comment 3 Benoit Jacob 2010-11-08 19:16:19 UTC
> The expression sin(A)+cos(B) will be evaluated 3 times ! Even if I copy xpr
> into a Xpr::Nested.

OK good point. Basically A and B should be evaluated if their access cost is any higher than the cost of access cost of a plain object.

> Even worse:
> 
> MatrixXf A,B;
> foo(A*B);
> 
> => this won't compile!

Ah right. Or if we allow it, it will use slow coeff-based product computation. Currently this is taken care of by Nested...

OK, your 2 examples show clearly that even after the evaluator changes, we'll still need to take care of function arguments if only from an optimization point of view.

> whence our paste discussion about adding an official NestedForMultipleAccess<>
> like stuff, and document it. But in 3.1 these official Nested* stuff won't make
> much sense since they be based on internal::nesting at all. and consequently it
> would be better to find a better yet consistent naming scheme for this
> mechanism ...

Indeed, Nested isn't the right name anymore. This is only a name problem, right?

If it were just about the MultipleAccess aspect, I'd say: AccessedMultipleTimes.

If it were just for Eigen 3.1, the evaluator would be what we want here.

Ah here's an idea: PlainObjectIfNeeded
well feel free to find a lighter name but I think that this summarizes what it is.

We can still:
 * keep the Nested typedefs around deprecated in 3.1
 * or find a better name for them
 * or tell the user to use const PlainObject& in Eigen 3.0 which means evaluate, just avoid doing a useless temporary if we already had a plain object.
 * or say that function taking xpr types are unsupported in 3.0.
Comment 4 Benoit Jacob 2010-11-09 00:10:27 UTC
Gael -- I wanted to add that I'm still open to the option of just fixing bug 99 now and be done with these issues. It's just that I'm not sure at all that I have time for it, and that having this kind of deep issues this late in the process suggests that it might be saner to just defer that stuff to the next version and find a way to make that OK with users (API-wise). But I have no big certitudes.
Comment 5 Benoit Jacob 2010-11-24 15:04:23 UTC
I'm moving this to a 3.1 target and filing a 3.0 bug about reworking the dox page for 3.0.
Comment 6 Gael Guennebaud 2012-03-26 18:04:31 UTC
clearly no time to handle that for 3.1
Comment 7 Christoph Hertzberg 2012-12-20 17:31:08 UTC
Parts of this can be handled by Bug 481.
Comment 8 Gael Guennebaud 2015-09-02 11:32:58 UTC
It is still not clear what to do for that, so let's postpone the decision until we have gained more insight about will be really needed with the evaluators.
Comment 9 Gael Guennebaud 2018-10-09 19:38:03 UTC
For the record, internally we do:

template<typename Derived>
void foo(const MatrixBase<Derived>& a_mat)
{
  typedef typename internal::nested_eval<Derived,2>::type Nested;
  Nested mat(a_mat.derived());
  ...
}

with '2' being the number of times 'mat' is used. It can be Dynamic to say a lot (3 is already a lot;) )

The question of making it public is still open.
Comment 10 Nobody 2019-12-04 09:58:37 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/111.