Re: [eigen] Enhanced AlignedBox

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

Hi Manuel,

thanks for the update,  but I still have a few remarks (I forgot some
in my previous email):

* for the implementation of sizes you can simply do:

return m_max - m_min;

that is much simpler. If you wonder, yes, the type of "m_max-m_min" is
"CwiseBinaryOp< ei_scalar_difference_op<Scalar>, VectorType,
VectorType>".


* for center(), in order to have zero temporary you have to nest
multiple expressions like this:

inline
const CwiseUnaryOp<ei_scalar_quotient1_op<Scalar>,
                                 CwiseBinaryOp<
ei_scalar_sum_op<Scalar>, VectorType, VectorType> > center() const
{
  return (m_min + m_max)/2;
}

* Why did you template the functions taking another aligned box.
(e.g., contains(), extend(), clamp(), etc.). I mean, both the
dimension and the scalar type have to be exactly the same, so no need
to template them.

* Likewise you templated the functions initially taking a VectorType
that is a good thing to avoid necessary copies when the actual
parameter is not exactly the same type (e.g., a Block expression).
However you must honor the "Nested" type of the expression. Let me
explain on this example:

template<typename Derived>
inline bool contains(const MatrixBase<Derived>& p) const
{ return (m_min.cwise()<=p).all() && (p.cwise()<=m_max).all(); }

Here "p" is used twice. So for instance if someone do:

MyAABB aabb(XX,YY);

aabb.contains((a+b)/2);

since the type of "(a+b)/2" is an expression of the sum of two vectors
divided by two, this expression will be evaluated twice by the
function contains that is not very good. This might be even worse if
the expression is more complicated like a product, or the solution of
a solver, e.g.:

aabb.contains(A.lu().solve(b));

The equation Ax=b will be solved twice! So the solution is to copy
each of such generic arguments to their respective ideal nesting type
according to the number of evaluation, e.g.:

template<typename Derived>
inline bool contains(const MatrixBase<Derived>& a_p) const
{
  const typename ei_nested<Derived,2>::type p(a_p.derived());
  return (m_min.cwise()<=p).all() && (p.cwise()<=m_max).all();
}

Here the "2" is because p is going to be evaluated twice. Eigen will
compare the cost of evaluating a_p twice to the cost of evaluating it
once to a temporary and then access twice to the temporary, and
according to the result of this comparison, typename
ei_nested<Derived,2>::type will be either a reference "Derived&" or,
in this case, the same as "VectorType".

side note:

Now I'm also thinking that this expression is not as efficient as it
could be because it would be more efficient to loop through p only
once using a ternary operators like:
p.cwise().isInRange(m_min,m_max).all(); but that's not currently
possible. Note that such an optimization would only makes sense for
large dimensions.


* finally it would be cool if you could configure your editor to use
two spaces instead of "tabs" for the indentation

gael






On Mon, Dec 21, 2009 at 9:10 PM, Manuel Yguel <manuel.yguel@xxxxxxxxx> wrote:
> Hi Gael,
> thanks for the useful remarks.
> I agree with all you said and I modify the code according to what you suggested.
>
> I am not at all familiar with how the expressions work in Eigen, so
> consider it a try.
> It seems to me that there is probably a simplest way to write the
> code, so correct me if there is one: I will learn.
>
> I am thinking of the same kind of design that the one you suggested
> for the random number generator.
> I like the interface of the boost random number generators, so I will
> try to make a proposition compatible with those ones.
>
>
> - best regards,
>
> Manuel
>