Bug #1474956 “Clump::updateProperties discretization not defined...” : Bugs : Yade

Bruno Chareyre (bruno-chareyre) on 2015-07-15

Changed in yade:
importance:	Undecided → Medium
assignee:	nobody → Bruno Chareyre (bruno-chareyre)
status:	New → Confirmed

Revision history for this message

Christian Jakob (jakob-ifgt) wrote on 2015-07-21:

#1

Hi,

I agree, that in some special cases aabb should be used for discretization, but...

... it should be downward compatible (user can decide if he/she want the discretization this way or this way ... so a new bool is needed in Clump::updateProperties: discretionAabb = false [default])
... it needs a lot of extra code modification (each function, that calls updateProperties directly or indirectly)
... it needs an update of clump section in the DEM background manual [1]

Regards,

Christian

[1] https://yade-dem.org/doc/formulation.html#clumps-rigid-aggregates

Revision history for this message

Bruno Chareyre (bruno-chareyre) wrote on 2015-07-21:

#2

>it should be downward compatible

Sure of that? It makes things more complex without adding any additional flexibility.
As far as inertia matters, there is no point in subdiscretizing this tiny sphere which is 1000 times smaller that the clump.
But if you really want to do it you can: discretization=10000.
It is not possible the other way around since discretization can't be less than 1 (as explained above).
What is the advantage of defining resolution wrt the smallest element of an object overall?

I agree that downward compatibility is good when possible, but we can't keep it at all prices. Sometimes we have to keep things simple to.

>it needs a lot of extra code modification

Without the above optional behavior the fix fits in 3 changed lines (and one change is in fact unrelated, see below).

Thanks for pointing to the relevant doc section.

Bruno

p.s. Please don't cc me or anybody else when sending email to bug/aswers/lists because it generates duplicates

diff --git a/core/Clump.cpp b/core/Clump.cpp
index deb10b5..869cc5a 100644
--- a/core/Clump.cpp
+++ b/core/Clump.cpp
@@ -144,17 +144,18 @@ void Clump::updateProperties(const shared_ptr<Body>& clumpBody, unsigned int dis
      */
        if(intersecting){
- //get boundaries and minimum radius of clump:
- Real rMin=1./0.; AlignedBox3r aabb;
+ //get boundaries of clump:
+ AlignedBox3r aabb;
                FOREACH(MemberMap::value_type& mm, clump->members){
                        const shared_ptr<Body> subBody = Body::byId(mm.first);
                        if (subBody->shape->getClassIndex() == Sph_Index){//clump member should be a sphere
                                const Sphere* sphere = YADE_CAST<Sphere*> (subBody->shape.get());
                                aabb.extend(subBody->state->pos + Vector3r::Constant(sphere->radius));
                                aabb.extend(subBody->state->pos - Vector3r::Constant(sphere->radius));
- rMin=min(rMin,sphere->radius);
                        }
                }
+ Real rMin=min(aabb.diagonal()[0],min(aabb.diagonal()[1],aabb.diagonal()[2]));
+
                //get volume and inertia tensor using regular cubic cell array inside bounding box of the clump:
                Real dx = rMin/discretization; //edge length of cell
                Real dv = pow(dx,3); //volume of cell
@@ -182,8 +183,7 @@ void Clump::updateProperties(const shared_ptr<Body>& clumpBody, unsigned int dis
                                }
                        }
                }
- }
- if(!intersecting){
+ } else {//not intersecting
                FOREACH(MemberMap::value_type& mm, clump->members){
                        // mm.first is Body::id_t, mm.second is Se3r of that body
                        const shared_ptr<Body> subBody=Body::byId(mm.first);

>it should be downward compatible

Sure of that? It makes things more complex without adding any additional flexibility.
As far as inertia matters, there is no point in subdiscretizing this tiny sphere which is 1000 times smaller that the clump.
But if you really want to do it you can: discretization=10000.
It is not possible the other way around since discretization can't be less than 1 (as explained above).
What is the advantage of defining resolution wrt the smallest element of an object overall?

I agree that downward compatibility is good when possible, but we can't keep it at all prices. Sometimes we have to keep things simple to.

>it needs a lot of extra code modification

Without the above optional behavior the fix fits in 3 changed lines (and one change is in fact unrelated, see below).

Thanks for pointing to the relevant doc section.

Bruno

p.s. Please don't cc me or anybody else when sending email to bug/aswers/lists because it generates duplicates

diff --git a/core/Clump.cpp b/core/Clump.cpp
index deb10b5..869cc5a 100644
--- a/core/Clump.cpp
+++ b/core/Clump.cpp
@@ -144,17 +144,18 @@ void Clump::updateProperties(const shared_ptr<Body>& clumpBody, unsigned int dis
      */
        if(intersecting){       
-               //get boundaries and minimum radius of clump:
-               Real rMin=1./0.; AlignedBox3r aabb;
+               //get boundaries of clump:
+               AlignedBox3r aabb;
                FOREACH(MemberMap::value_type& mm, clump->members){
                        const shared_ptr<Body> subBody = Body::byId(mm.first);
                        if (subBody->shape->getClassIndex() == Sph_Index){//clump member should be a sphere
                                const Sphere* sphere = YADE_CAST<Sphere*> (subBody->shape.get());
                                aabb.extend(subBody->state->pos + Vector3r::Constant(sphere->radius));
                                aabb.extend(subBody->state->pos - Vector3r::Constant(sphere->radius));
-                               rMin=min(rMin,sphere->radius);
                        }
                }
+               Real rMin=min(aabb.diagonal()[0],min(aabb.diagonal()[1],aabb.diagonal()[2])); 
+               
                //get volume and inertia tensor using regular cubic cell array inside bounding box of the clump:
                Real dx = rMin/discretization;  //edge length of cell
                Real dv = pow(dx,3);            //volume of cell
@@ -182,8 +183,7 @@ void Clump::updateProperties(const shared_ptr<Body>& clumpBody, unsigned int dis
                                }
                        }
                }
-       }
-       if(!intersecting){
+       } else {//not intersecting
                FOREACH(MemberMap::value_type& mm, clump->members){
                        // mm.first is Body::id_t, mm.second is Se3r of that body
                        const shared_ptr<Body> subBody=Body::byId(mm.first);

Revision history for this message

Christian Jakob (jakob-ifgt) wrote on 2015-07-21:

#3

>>it should be downward compatible

>Sure of that?

No. I just thought about users, that already use discretization option to get mass of clumps (like me ...)
If the users now use the same script with new yade version, it will give different results (maybe unnoticed by the user).

>As far as inertia matters, there is no point in subdiscretizing this tiny sphere which is 1000 times smaller that the clump.
>But if you really want to do it you can: discretization=10000.

Yes I see the problem and I agree with the change in code, but please give a note for the users!

Now to the doc ... After a closer look I think there should be no change in [1] since it just says that aabb is discretized (but not how). There should be an update of [2] instead.

Also it needs an update of the examples/clumps section ...

>p.s. Please don't cc me or anybody else when sending email to bug/aswers/lists because it generates duplicates

I did not cc anyone. This seems to be a bug in launchpad mail delivery system. I get double-posts nearly every day.

[1] see above
[2] https://yade-dem.org/doc/yade.wrapper.html?highlight=clump#yade.wrapper.BodyContainer.clump

Bruno Chareyre (bruno-chareyre) on 2018-05-21

Changed in yade:
importance:	Medium → Wishlist

Revision history for this message

Klaus Thoeni (klaus.thoeni) wrote on 2018-05-21: Re: [Yade-dev] [Bug 1474956] Re: Clump::updateProperties discretization not defined robustly

#4

Sounds like a good fix to me.

Klaus

On Tue, May 22, 2018 at 7:11 AM, Bruno Chareyre <email address hidden>
wrote:

> ** Changed in: yade
> Importance: Medium => Wishlist
>
> --
> You received this bug notification because you are a member of Yade
> developers, which is subscribed to Yade.
> https://bugs.launchpad.net/bugs/1474956
>
> Title:
> Clump::updateProperties discretization not defined robustly
>
> Status in Yade:
> Confirmed
>
> Bug description:
> Most likely the reason for performance drop reported in [1]
>
> dx in the voxel decomposition is minRad/discretization.
> The problem is when a sphere of radius 1 is member of a clump which aabb
> has the size 1000 (for instance).
> Using the minimal discretization=1 gives a number of voxel = 1e9 in such
> case and it is no possible to go below this number.
> I think discretization should refer to aabb size, not member size. I
> have this fixed locally, let me know if you agree with the fix.
>
> [1] https://answers.launchpad.net/yade/+question/268954
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/yade/+bug/1474956/+subscriptions
>
> _______________________________________________
> Mailing list: https://launchpad.net/~yade-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~yade-dev
> More help : https://help.launchpad.net/ListHelp
>

Revision history for this message

Bruno Chareyre (bruno-chareyre) wrote on 2018-05-22:

#5

So old that I didn't bother getting back to it, but if someone wants to implement it... hence the wishlist :)

Revision history for this message

Janek Kozicki (cosurgi) wrote on 2019-02-05:

#6

moved from https://bugs.launchpad.net/yade/+bug/1474956 to https://gitlab.com/yade-dev/trunk/issues/11

Yade

Clump::updateProperties discretization not defined robustly

Bug Description

Other bug subscribers

Remote bug watches