Re: [eigen] Enhanced AlignedBox |

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

*To*: eigen@xxxxxxxxxxxxxxxxxxx*Subject*: Re: [eigen] Enhanced AlignedBox*From*: Gael Guennebaud <gael.guennebaud@xxxxxxxxx>*Date*: Mon, 21 Dec 2009 11:13:12 +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 :date:message-id:subject:from:to:content-type; bh=Fvv5830snDTSbGj3/7+hFUcbj5g0Kx30Cl7OWyojgwM=; b=IB+ehSKhPEYpNTiv8sdJys4JLbGWk7m4TzGJGgDMJclntLrDPS+H8cmpUfw1ElKLhR hD1YihY+YO3K1xajZjIIY0zJ6yaFaZWbt4SYXdMyAXiAbpKosmCQNWsXffUP9jgu6hNa O3gRqMeJzrZPe8a9FM+sEBWqiApNilyltwUog=*Domainkey-signature*: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=rfGdoxFqCLyFE3aAi7g+PsYPkwjSw/KJIz5+tp1hlBec2hOh2XK6HkpPMP3m3qBxKv fmfel8VhwNzJVhmcMkyLGf9A9GEW/zXIHrKE9paN2Xer31gcKyWmexylZUI5zweiEQTR cWu18M4B9+eRDB7gFbye4LJy9ILivWYTfjSNU=

** sorry, I sent the previous email by mistake, please ignore it. **

Hi Manuel,

thanks a lot for your propositions and the patch. I have some remarks though:

Note that in the patch they are called side and sides, and I do prefer size/sizes. Also I'm thinking that size() in not needed. Indeed, if we re-implement sizes() to return an _expression_ rather than a vector, then box.sizes()[i] we'll be equivalent to size(i).

Likewise, if we should re-implement center() to return an _expression_ so that the center(int) overload is not needed.

In the same vein, minSize() is only shortcut for .sizes().minCoeff() that is not much longer and at least as explicit, so I'm not sure these two shortcuts (minSize/maxSize) are really needed. Note that though the volume() function is only a shortcut for "sizes().prod()", it really adds a semantic value, so it's fine to keep this one.

why not but we should provide nice names for each dimension, I mean BottomLeft/BottomRight does not really make sens for a 1D AABB. So what about adding some aliases:

- Min Max for 1D,

- and *Floor versions for 3D.

diagonal() should rather return an _expression_ of the diagonal vector (=> alias for sizes(), at least for floats), and then these two functions should rather be renamed diagonalNorm() and squaredDiagonalNorm(), and then we see they are not really needed because they do not add any value compared to: diagonal().norm() diagonal().squaredNorm(.

I don't care much, if someone else has an opinion...

agree.

Well, as you said that depends on the semantic we give to our bounding box, and to the size (side) function. Should it return the number of elements or the length? Since it is called a *bounding* box, the min and max represent bounds, and so it make more sense to me to return the length, so min-max. For instance, an AABB of integers is useful, to represent a region in, e.g., a 3D grid. Then the min() and max() represents the indices of the respective corner, and max()-min() represents the number of cells in each dimension, and so it makes sense to make sizes() returns max()-min(). Moreover, this simplify the implementation and avoid an inconsistent behavior.

In that case, note that diagonal() == size()

ok, I'll check the generated code (paranoia)

why not.

We did not discussed about that yet, so feel free to start one if you have some ideas, but basically the way I see it is to add to the functions relying on random numbers a functor parameters defaulting to a functor wrapping ei_random(). For instance for the sample function you wrote we could do:

template<typename RNG> VectorType sample(const RNG& rng) const { /* ... */ };

VectorType sample() const { return sample(DefaultRNG()) };

gael

On Sun, Dec 20, 2009 at 11:30 PM, Manuel Yguel <manuel.yguel@xxxxxxxxx> wrote:

Hello,

I have written some small pieces of codes to enhance AlignedBox.

The following changes I propose are as follow:

thanks a lot for your propositions and the patch. I have some remarks though:

1) Define some new functions:

Scalar size( size_t k ) --> returns the size of the k-th side

Vector sizes() --> returns a vector with all the sides

Note that in the patch they are called side and sides, and I do prefer size/sizes. Also I'm thinking that size() in not needed. Indeed, if we re-implement sizes() to return an _expression_ rather than a vector, then box.sizes()[i] we'll be equivalent to size(i).

Likewise, if we should re-implement center() to return an _expression_ so that the center(int) overload is not needed.

In the same vein, minSize() is only shortcut for .sizes().minCoeff() that is not much longer and at least as explicit, so I'm not sure these two shortcuts (minSize/maxSize) are really needed. Note that though the volume() function is only a shortcut for "sizes().prod()", it really adds a semantic value, so it's fine to keep this one.

Vector corner( CornerType t ) --> defines some enum and return the

corresponding corner for 1D,2D and 3D boundingBox

why not but we should provide nice names for each dimension, I mean BottomLeft/BottomRight does not really make sens for a 1D AABB. So what about adding some aliases:

- Min Max for 1D,

- and *Floor versions for 3D.

Scalar squaredDiagonale()

Real diagonal()

diagonal() should rather return an _expression_ of the diagonal vector (=> alias for sizes(), at least for floats), and then these two functions should rather be renamed diagonalNorm() and squaredDiagonalNorm(), and then we see they are not really needed because they do not add any value compared to: diagonal().norm() diagonal().squaredNorm(.

Scalar volume()

Vector sample() --> sample a point in the boundingBox volume with a

uniform distribution

2) Add the function isEmpty (I do not like very much the isNull name,

it is a matter of taste, so if it does not fit you, let me know...)

I don't care much, if someone else has an opinion...

3) Semantic: I think that it is usefull to have bounding boxes with

integers: in this case the min and the max are considered inside the

bounding box

agree.

and a side of the bounding box is max - min + 1 instead

of max - min.

This is a complex semantic problem, but I think it is the more natural choice.

Well, as you said that depends on the semantic we give to our bounding box, and to the size (side) function. Should it return the number of elements or the length? Since it is called a *bounding* box, the min and max represent bounds, and so it make more sense to me to return the length, so min-max. For instance, an AABB of integers is useful, to represent a region in, e.g., a 3D grid. Then the min() and max() represents the indices of the respective corner, and max()-min() represents the number of cells in each dimension, and so it makes sense to make sizes() returns max()-min(). Moreover, this simplify the implementation and avoid an inconsistent behavior.

In that case, note that diagonal() == size()

4) Following that line, I have changed the code for the distances

functions such that non-negative integers types are safely handled and

I trust the compiler to produce as optimized code as the one that Gael

has written.

I know that such types are not natively handled by Eigen, but I think

that it is a safe change.

ok, I'll check the generated code (paranoia)

What do you think about those proposals?

If you agree with that lines, I plan to write an OrientedBox class

with almost same interface.

why not.

Also, I would like to launch a topic about the way random numbers are

generated because when writing sample(), I found that we can enhance

the way it is done.

Is this already an active topic ?

We did not discussed about that yet, so feel free to start one if you have some ideas, but basically the way I see it is to add to the functions relying on random numbers a functor parameters defaulting to a functor wrapping ei_random(). For instance for the sample function you wrote we could do:

template<typename RNG> VectorType sample(const RNG& rng) const { /* ... */ };

VectorType sample() const { return sample(DefaultRNG()) };

gael

- best regards,

Manuel

**Follow-Ups**:**Re: [eigen] Enhanced AlignedBox***From:*Manuel Yguel

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

**Messages sorted by:**[ date | thread ]- Prev by Date:
**Re: [eigen] Enhanced AlignedBox** - Next by Date:
**Re: [eigen] MSVC debug tools** - Previous by thread:
**Re: [eigen] Enhanced AlignedBox** - Next by thread:
**Re: [eigen] Enhanced AlignedBox**

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