Loading clumps takes too much time

[Anton Gladky]

The examples/packs/packs.py hangs in line 46 [1] because
of appendClumped command.

Changing it to "append" fixes the problem. So, it seems,
appendClump has infinite loop somewhere.

[1] https://github.com/yade/trunk/blob/master/examples/packs/packs.py#L46

[Anton Gladky (gladky-anton)]

To be more precise, the infinite loop is here [1].
Christian, please, have a look.

Thanks,

[1] https://github.com/yade/trunk/blob/master/core/Clump.cpp#L162

Anton

[Bruno Chareyre (bruno-chareyre)]

May I ask how you found the precise location of the infinite loop Anton?
I would not know how to spot such things.
In the present case, it is not clear after reading the code how it can
go infinite. I hope Christian will have an idea.

Bruno

[Anton Gladky (gladky-anton)]

Just std::cerr in many places, then you can localize the problem.

Anton

2013/9/24 Bruno Chareyre <email address hidden>:
> May I ask how you found the precise location of the infinite loop Anton?
> I would not know how to spot such things.
> In the present case, it is not clear after reading the code how it can
> go infinite. I hope Christian will have an idea.
>
> Bruno
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1229783
>
> Title:
> packs.py example hangs due to appendClumped command
>
> Status in Yet Another Dynamic Engine:
> New
>
> Bug description:
> The examples/packs/packs.py hangs in line 46 [1] because
> of appendClumped command.
>
> Changing it to "append" fixes the problem. So, it seems,
> appendClump has infinite loop somewhere.
>
> [1]
> https://github.com/yade/trunk/blob/master/examples/packs/packs.py#L46
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/yade/+bug/1229783/+subscriptions

[Christian Jakob (jakob-ifgt)]

Hm, i had a short look at the code, but do not see why it can get infinite ...

When i am back to germany (not before next tuesday) i will try solve it.

thanks for reporting, anton.

c

[Anton Gladky (gladky-anton)]

Christian,

It seems, the problem commit is this one [1]. There was a "break" in
line 275 [2], but it dissapepared in a new version of Clump.cpp [3].

I did not analyze the logic, but this commit should be double-checked,
as it causes such strange behavior.

Cheers,

[1] https://github.com/yade/trunk/commit/1e5286018b522f9f470799c6f5304886b74a427d
[2] https://github.com/yade/trunk/commit/1e5286018b522f9f470799c6f5304886b74a427d#L0L275
[3] https://github.com/yade/trunk/commit/1e5286018b522f9f470799c6f5304886b74a427d#L0R169

Anton

2013/9/25 Christian Jakob <email address hidden>:
> Hm, i had a short look at the code, but do not see why it can get
> infinite ...
>
> When i am back to germany (not before next tuesday) i will try solve it.
>
> thanks for reporting, anton.
>
> c
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1229783
>
> Title:
> packs.py example hangs due to appendClumped command
>
> Status in Yet Another Dynamic Engine:
> New
>
> Bug description:
> The examples/packs/packs.py hangs in line 46 [1] because
> of appendClumped command.
>
> Changing it to "append" fixes the problem. So, it seems,
> appendClump has infinite loop somewhere.
>
> [1]
> https://github.com/yade/trunk/blob/master/examples/packs/packs.py#L46
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/yade/+bug/1229783/+subscriptions

[Anton Gladky (gladky-anton)]

Sorry for false alarm. It works, but takes some time to be initialized....

Anton

[Bruno Chareyre (bruno-chareyre)]

A question for Christian:
If I have a clump with, let's say, 20 particles. Will there be 20
computations of inertia (each time we add a new member) or only one?
We should find a default behavior that do not take ages to compute
anyway, be it at the price of big approximations.

Bruno

[Klaus Thoeni (klaus.thoeni)]

Hi guys

loading a clump takes ages indeed (I have clumps with 100 particles) but not
sure if this is a bug. Maybe there is something wrong with the loops?

Klaus

On Thursday 26 September 2013 08:55:19 Bruno Chareyre wrote:
> A question for Christian:
> If I have a clump with, let's say, 20 particles. Will there be 20
> computations of inertia (each time we add a new member) or only one?
> We should find a default behavior that do not take ages to compute
> anyway, be it at the price of big approximations.
>
> Bruno
>
> --
> You received this bug notification because you are a member of Yade
> developers, which is subscribed to Yade.
> https://bugs.launchpad.net/bugs/1229783
>
> Title:
> packs.py example hangs due to appendClumped command
>
> Status in Yet Another Dynamic Engine:
> Invalid
>
> Bug description:
> The examples/packs/packs.py hangs in line 46 [1] because
> of appendClumped command.
>
> Changing it to "append" fixes the problem. So, it seems,
> appendClump has infinite loop somewhere.
>
> [1]
> https://github.com/yade/trunk/blob/master/examples/packs/packs.py#L46
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/yade/+bug/1229783/+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

--
Dr. Klaus Thoeni - Centre for Geotechnical and Materials Modelling
Civil, Surveying and Environmental Engineering - Engineering Building EA
The University of Newcastle, Callaghan, NSW 2308, Australia
web: http://www.newcastle.edu.au/research-centre/cgmm
phone: [phone]

[Christian Jakob (jakob-ifgt)]

> If I have a clump with, let's say, 20 particles. Will there be 20
> computations of inertia (each time we add a new member) or only one?

Bruno, as you know there are several ways of creating/modifying a clump.

Each call of Clump::updateProperties(clumpBody);
will cause approximation of inertia and this takes some time...

updateProperties is called in following methods:

- clump() (see [1])
- appendClumped() (which calls clump() method)
- addToClump() (see [2])
- releaseFromClump() (see [3]) and
- replaceByClumps (which also calls clump() method)

If you use addToClump() method it is possible to add more than one body via python list. The updateProperties method will be called once per addToClump() call.

[1] https://github.com/yade/trunk/blob/master/py/wrapper/yadeWrapper.cpp#L152
[2] https://github.com/yade/trunk/blob/master/py/wrapper/yadeWrapper.cpp#L182
[3] https://github.com/yade/trunk/blob/master/py/wrapper/yadeWrapper.cpp#L197

> We should find a default behavior that do not take ages to compute
> anyway, be it at the price of big approximations.

I agree. As you can see here [4] there is a TODO. Edge length of one cell for approximation of inertia tensor is defined as minimum clump member radius divided by divisor (actually 15). To avoid worst case of a clump with one very small member a maximum number of cells is defined [5]. The maximum number of cells is limited to 150^3 = 3.375 million.

If we change default of divisor (in woo it is 5), the accuracy of inertia approx. will be lower but the scheme would be much faster. I think the best way would be to make it choosable for users (see TODO).
I can do it with some help from dev-side. Where and can divisor be defined and choosable by users (NOTE that updateProperties is called by 5 methods (see above)!)

[4] https://github.com/yade/trunk/blob/master/core/Clump.cpp#L153
[5] https://github.com/yade/trunk/blob/master/core/Clump.cpp#L156

> loading a clump takes ages indeed (I have clumps with 100 particles) but not
> sure if this is a bug. Maybe there is something wrong with the loops?

Klaus, would you agree with my suggestions? Then you could choose accuracy/speed of inertia approximation by yourself ;)

[Anton Gladky (gladky-anton)]

I maybe wrong, sorry, never coded on "clumps".
But, it seems, the bottleneck is the loop in lines 159-177 [1].

[1] https://github.com/yade/trunk/blob/master/core/Clump.cpp#L159

Why do we need to discretize the cell? Is it possible just to go through all
spheres and to summ m*r^2 of each sphere?

Anton

2013/10/2 Christian Jakob <email address hidden>:
> Klaus, would you agree with my suggestions? Then you could choose
> accuracy/speed of inertia approximation by yourself ;)

[Christian Jakob (jakob-ifgt)]

> Why do we need to discretize the cell? Is it possible just to go through all
> spheres and to summ m*r^2 of each sphere?

Yes and no.

For non-intersecting clump members,it is done this way (see Clump.cpp lines 179 - 190).

For intersecting clump member, it will be wrong, because masses are considered multiple for intersecting parts.

[Anton Gladky (gladky-anton)]

2013/10/2 Christian Jakob <email address hidden>:
> For intersecting clump member, it will be wrong, because masses are
> considered multiple for intersecting parts.

Why then the loop FOREACH (line 162) does not stop after the first
sphere has been found? If 2 or 3 spheres are interacting, that the moment
will be calculated 2 or 3 times?

[Klaus Thoeni (klaus.thoeni)]

Hi Christian

On Wednesday 02 October 2013 07:18:28 Christian Jakob wrote:
> > If I have a clump with, let's say, 20 particles. Will there be 20
> > computations of inertia (each time we add a new member) or only one?
>
> Bruno, as you know there are several ways of creating/modifying a clump.
>
> Each call of Clump::updateProperties(clumpBody);
> will cause approximation of inertia and this takes some time...
>
> updateProperties is called in following methods:
>
> - clump() (see [1])
> - appendClumped() (which calls clump() method)
> - addToClump() (see [2])
> - releaseFromClump() (see [3]) and
> - replaceByClumps (which also calls clump() method)
>
> If you use addToClump() method it is possible to add more than one body
> via python list. The updateProperties method will be called once per
> addToClump() call.
>
> [1]
> https://github.com/yade/trunk/blob/master/py/wrapper/yadeWrapper.cpp#L152
> [2]
> https://github.com/yade/trunk/blob/master/py/wrapper/yadeWrapper.cpp#L182
> [3]
> https://github.com/yade/trunk/blob/master/py/wrapper/yadeWrapper.cpp#L197

Are you sure that updateProperties is not called for each sphere in the clump
list? This means you would loop over the grid and all spheres for each sphere.

BTW, the loop over the grid could probably be parallelised with some
#pragma omp parallel for reduction(+:M,Sg,Ig).

> > We should find a default behavior that do not take ages to compute
> > anyway, be it at the price of big approximations.
>
> I agree. As you can see here [4] there is a TODO. Edge length of one
> cell for approximation of inertia tensor is defined as minimum clump
> member radius divided by divisor (actually 15). To avoid worst case of a
> clump with one very small member a maximum number of cells is defined
> [5]. The maximum number of cells is limited to 150^3 = 3.375 million.
>
> If we change default of divisor (in woo it is 5), the accuracy of inertia
> approx. will be lower but the scheme would be much faster. I think the best
> way would be to make it choosable for users (see TODO). I can do it with
> some help from dev-side. Where and can divisor be defined and choosable by
> users (NOTE that updateProperties is called by 5 methods (see above)!)
>
> [4] https://github.com/yade/trunk/blob/master/core/Clump.cpp#L153
> [5] https://github.com/yade/trunk/blob/master/core/Clump.cpp#L156
>
> > loading a clump takes ages indeed (I have clumps with 100 particles) but
> > not sure if this is a bug. Maybe there is something wrong with the loops?
> Klaus, would you agree with my suggestions? Then you could choose
> accuracy/speed of inertia approximation by yourself ;)

Of course, that would be nice, something like appendClumped(listOfBodies,
divisor). I was actually about to have a look how to implement it as well. So
let me know if you are doing it.

Klaus

[Christian Jakob (jakob-ifgt)]

> Why then the loop FOREACH (line 162) does not stop after the first
> sphere has been found?

because you found a bug!
^^

thanks anton, it is hopefully fixed here:

https://github.com/yade/trunk/commit/4f66770c9fdd2bf9e2af07a7bfe690a7d4ad9be7

> BTW, the loop over the grid could probably be parallelised with some
> #pragma omp parallel for reduction(+:M,Sg,Ig).

If your C++ skills are high enough, please do it!

> Of course, that would be nice, something like appendClumped(listOfBodies,
> divisor). I was actually about to have a look how to implement it as well. So
> let me know if you are doing it.

It is not that easy, because updateProperties is called by 5 methods, not just appendClumped (see above)!

divisor must be defined globally somewhere in the code (i do not know where is a good spot). Also we can change the default value (actually 15) if you think it is too high.

[Christian Jakob (jakob-ifgt)]

fixed in

https://github.com/yade/trunk/commit/8202468e7b483c936dbd099b2b03b79ee7150ab4

see

https://github.com/yade/trunk/commit/2987365a852e7a0a2e0ba98b95262fcfc8b6d9fb

[Bruno Chareyre (bruno-chareyre)]

Fixes have to be marked released, else the bugs appears as "open bug".