Re: [eigen] Enhanced AlignedBox |

[ Thread Index | Date Index | More lists.tuxfamily.org/eigen Archives ]

*To*: eigen@xxxxxxxxxxxxxxxxxxx*Subject*: Re: [eigen] Enhanced AlignedBox*From*: Manuel Yguel <manuel.yguel@xxxxxxxxx>*Date*: Wed, 23 Dec 2009 19:47:35 +0100*Dkim-signature*: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :from:date:message-id:subject:to:content-type :content-transfer-encoding; bh=Y9q0SYY0OCgHuFNasBrZxrIqv+0tPqwNtyLx9XNlg4U=; b=ZfUayFrom/QtIDrlT0PEZLSe44CCPrfR/y1nhQ6j7haWViLdU/e/34FXpmFIaW8jtg hVD0FQeaK+p7i1PE7Y0OP9JE5pr+2IHjAKfIhNV2I+ITSi8RmICiAQ51yskOlopoRHmp Xbz6qpolH7e71uSNurbWX7dpqw634SvBc7sPw=*Domainkey-signature*: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :content-type:content-transfer-encoding; b=Sq6fOrvIZBksKiEWjJzN2CVOps4aihBgFSq8AfyaMkwyzrQIjfNwX/yYfxzd6Iw768 WbS515b0xIZ8FyoD8NIBgtYQhZUEleonB4hTFNMvZV2VK0VlkMEUKYhQ8JEg/UTItxmU XFpokOdr1LfbWb5hYHSMHhfxLyS78KK/tfCZM=

On Tue, Dec 22, 2009 at 2:48 PM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote: > 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; > } Thanks I learn a lot there. > > * 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. Why? I agree for the dimension, but not necessary for the scalar type. One can want float for hardware reason and double for precision in an other part of the application. > > * 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". oh, oh that is a great tool. > > 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 My mistake. > > gael > > Here is the last problem I see, this kind of code: m_min.setConstant( std::numeric_limits<Scalar>::max()); m_max.setConstant(-std::numeric_limits<Scalar>::max()); is valid for floating types however it is not true for general types so usually I prefer to use the boost definitions, defined in the following header: #include <boost/numeric/conversion/bounds.hpp> m_min.setConstant( boost::numeric::bounds<float>::highest() ); m_max.setConstant( boost::numeric::bounds<float>::lowest() ); Do you think it is the eigen way to add some fields and/or static methods to NumTraits in order to provide the equivalent following informations? In the std::numeric_limits they give also minimum positive number, precision, codes for NAN, +INF etc... I saw that you define dummy_precision as a template function, should it not be a static method part of the NumTraits such that someone can easily change the default behaviour by defining its own NumTraits ? If you agree with that I would make the changes adding the static functions: highest() lowest() dummy_precision() adding the value for -INF also etc... and after we can mimic std::numeric_limits epsilon() has_infinity() has_quiet_NaN() has_signaling_NaN() etc ... it should be mere copy-paste from the stl + boost (a bit boring I agree, but I can do it) - best regards and merry christmas Manuel

**References**:**[eigen] Enhanced AlignedBox***From:*Manuel Yguel

**Re: [eigen] Enhanced AlignedBox***From:*Gael Guennebaud

**Re: [eigen] Enhanced AlignedBox***From:*Manuel Yguel

**Re: [eigen] Enhanced AlignedBox***From:*Gael Guennebaud

**Messages sorted by:**[ date | thread ]- Prev by Date:
**Re: [eigen] Enhanced AlignedBox** - Next by Date:
**[eigen] 2.0.11 on January 10 ?** - Previous by thread:
**Re: [eigen] Enhanced AlignedBox** - Next by thread:
**[eigen] 2.0.11 on January 10 ?**

Mail converted by MHonArc 2.6.19+ | http://listengine.tuxfamily.org/ |