Swap problem in InteractionLoop

My next questions (result of not knowing the code well enough):
- is asssert necessary in [1]?
- why is IPhysDispatcher defined as autosymmetric (default template argument) [3]?

[1] http://bazaar.launchpad.net/~yade-pkg/yade/git-trunk/view/head:/pkg/common/InteractionLoop.cpp#L116
[3] http://bazaar.launchpad.net/~yade-pkg/yade/git-trunk/view/head:/pkg/common/Dispatching.hpp#L113

I don't understand this assert either.
Just adding more questions:
-Can the same problem also appear in Ip2_FrictMat_CpmMat_FrictPhys?
-This one requires that mat1 is Frict and Mat2 is Cpm, how can it be always true? [4]

The good fix would be to let materials be swapped, I don't believe it really needs two different functors.

[4] http://bazaar.launchpad.net/~yade-pkg/yade/git-trunk/view/head:/pkg/dem/ConcretePM.cpp#L25

This situation is partly described in [2]. I used it just in a special case:
- spheres with CpmMat
- facets with FrictMat
- Ig2_Facet_Sphere
- Ip2_FrictMat_CpmMat

so Ig2 always order the particles with different shapes in order
Facet-Sphere, so the materials are also ordered FrictMat-CpmMat fitting
perfectly the Ip2 definition :-) this was done "correctly" by luck, so I
did not have the discussed problem in this case..

If I used CpmMat for facets and FrictMat for spheres, or used CpmMat and
FrictMat for spheres only, I would have had this error..

cheers
Jan

2015-07-23 12:36 GMT+02:00 Bruno Chareyre <email address hidden>:

> I don't understand this assert either.
> Just adding more questions:
> -Can the same problem also appear in Ip2_FrictMat_CpmMat_FrictPhys?
> -This one requires that mat1 is Frict and Mat2 is Cpm, how can it be
> always true? [4]
>
> The good fix would be to let materials be swapped, I don't believe it
> really needs two different functors.
>
> [4] http://bazaar.launchpad.net/~yade-pkg/yade/git-
> trunk/view/head:/pkg/dem/ConcretePM.cpp#L25
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1475844
>
> Title:
> Swap problem in InteractionLoop
>
> Status in Yade:
> New
>
> Bug description:
> When you use two different materials, it might happen that
> IntreactionLoop ends with error [1]. It is because of IPhys stage (see
> also the comment in [1], but in reality Ip2 can by non-symmetric).
>
> Possible solutions:
> 1) define both Ip2_Mat1_Mat2 and Ip2_Mat2_Mat1 (according to [2] maybe
> does not work..)
> 2) replace assert [1] (something like if (swap) {
> I->functorCase.phys=physDispatcher->getFunctor2D(b2->material,b1->material,swap);
> swap=false }). Is the assert necessary at all?
> 3) make option 1) somehow automatic (using some macro?)
>
> Pros and cons:
> 1) would mess the source code and documentation a bit, but otherwise I
> think it is ok
> 2) would it have some side effects? Is it ok for Law2 stage? etc etc?
> 3) maybe the best option if the solution is reasonable
>
> cheers
> Jan
>
> [1]
> http://bazaar.launchpad.net/~yade-pkg/yade/git-trunk/view/head:/pkg/common/InteractionLoop.cpp#L116
> [2] https://answers.launchpad.net/yade/+question/269315
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/yade/+bug/1475844/+subscriptions
>

Hi,
> My next questions (result of not knowing the code well enough):
> - is asssert necessary in [1]?
Not really necessary. It just means that goReverse would have to be
implemented properly, which, back then, was not the case with any of the
existing Ip2 functors. With swap==true, you'd have to call goReverse
instead of go (line 122), and also remember along with the cached
functor object, that goReverse must be called subsequently (in next
steps) as well.

In Woo, I removed this functor caching, since it does not seem to bring
any benefit performance-wise.
> - why is IPhysDispatcher defined as autosymmetric (default template argument) [3]?
The two arguments are materials (same type), and their order is
"arbitrary" (depending on geometry), hence it is desirable that both
orderings are handled by the functor. I have yet to see an Ip2 functor
which properly implements goReverse.

Hope this makes sense.

Vaclav

Thank Vacalv,
So the two-step solution is:
- Implement goReverse (i.e. exactly the "Ip2_Mat1_Mat2/Ip2_Mat2_Mat1" pair but in a single functor.
- enable swap and remember it somehow (bool InteractionPhysics::swapped maybe?)

@Vaclav
>I removed this functor caching, since it does not seem to bring any benefit performance-wise.

IIRC the change [1] gave a significant (clearly measurable) speedup.
Before the change it was like this:

if (noIGfuntor || noIPfunctor) assignIGfunctor();
if (!IGfunctor.go()) continue;
assignIPfunctor();

For virtual interactions (large majority, typically) it was assigning IGfunctor again and again, at each iteration.
I don't know in which circumstances you made the comparison but it could be that the gain from caching was hidden by this small bug.
Ultimately, I guess queuing separately the interactions which have or do not have functor would be better, since it would remove a "if".
It reminds me a discussion with Colin Thornton when he said: "they invented vector processing, the BIG thing, then they realized that our programs contain "if" statements, F##@!!"
B

[1] https://github.com/yade/trunk/commit/15182aca8473be86c74b139731dbb88b0007f759#diff-726aef96982e392072499e825cf634b4L106

Hi Bruno,

I think the two-step solution is right as far as I can see.

For the performance, I apologize for my incorrect statement; you're right about this. The logic is a bit different in Woo (https://github.com/eudoxos/woodem/blob/master/pkg/dem/ContactLoop.cpp) and I will check again what is happening for contacts of undefined shape combination there (those should be minimized via particle masks, but who knows).

What I meant is that IIRC the dispatcher lookup is so fast that IIRC it brings very little to cache the result of the lookup as opposed to calling the dispatcher directly (in normal circumstances, not when it repeatedly fails, as in your examples).

Cheers, v.

Yes, I understand the idea on performance.
"table[i][j]->" is nearly as fast a "p->".
It makes sense but it is pitty, it was quite a bit of work to implement
the cache thing everywhere wasn't it? :)
Anyway, it would be interesting to have timings again.

B

> Yes, I understand the idea on performance.
> "table[i][j]->" is nearly as fast a "p->".
There an extra check that table element is something, but, as you say,
it is very quick.
> It makes sense but it is pitty, it was quite a bit of work to implement
> the cache thing everywhere wasn't it? :)
It is sometimes like that :)

v

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