bodyNumInteractionsHistogram broken

[Jérôme Duriez]

See https://answers.launchpad.net/yade/+question/676284 and:

#####################
O.bodies.append(sphere((0,0,0),1,dynamic=False))
O.bodies.append(sphere((0,0,1.9),1,dynamic=False))

O.step()

print 'BodyNumInteractions Histogram', bodyNumInteractionsHistogram(aabbExtrema())
####################

which returns (with trunk version) :

ArgumentError: Python argument types in
    yade._utils.bodyNumInteractionsHistogram(list)
did not match C++ signature:
    bodyNumInteractionsHistogram(boost::python::tuple aabb, bool contactOnly=False)

because my commit [*] changed the Python return type of aabbExtrema() from tuple to list.

Possible (and easiest) fix would be to change all corresponding py::tuple aabb from py::list aabb. Agree ?

Or do we want this function to deal with Python tuples and not lists ?..

Jérôme

[*] https://github.com/yade/trunk/commit/1db13fb1183b9e294dc9761da76cfa4fc2791cc1

[Robert Caulk (rcaulk)]

It seems the only option is to go through and fix these functions manually. Let's just make sure we find everything that needs to be fixed before committing the fix. Can you verify the list below and also look through the trunk to see if there are any other unintended effects?

Looking through trunk/utils.py it seems the following functions need to be fixed:
uniaxialTestFeatures()[1]
aabbDim() [2]
plotNumInteractions() [3]
plotDirections() [4]
avgNumInteractions() [7]

Meanwhile, the flexibility of the python language means that the following functions should work before and after the change, right? :
fractionalBox()[5]
perpindicularArea()[6]

Cheers,

Robert

[*]https://bugs.launchpad.net/yade/+bug/1764424
[1]https://github.com/yade/trunk/blob/720a352989c9b1148cf04022d14ec3da233fe563/py/utils.py#L558
[2]https://github.com/yade/trunk/blob/720a352989c9b1148cf04022d14ec3da233fe563/py/utils.py#L379
[4]https://github.com/yade/trunk/blob/720a352989c9b1148cf04022d14ec3da233fe563/py/utils.py#L448
[3]https://github.com/yade/trunk/blob/720a352989c9b1148cf04022d14ec3da233fe563/py/utils.py#L459
[5]https://github.com/yade/trunk/blob/720a352989c9b1148cf04022d14ec3da233fe563/py/utils.py#L402
[6]https://github.com/yade/trunk/blob/720a352989c9b1148cf04022d14ec3da233fe563/py/utils.py#L393
[7]https://github.com/yade/trunk/blob/720a352989c9b1148cf04022d14ec3da233fe563/py/utils.py#L419

[Jérôme Duriez (jduriez)]

See commit https://github.com/yade/trunk/commit/7c60d78bd9badfa174fb79a82af7b119abf81d5d. It fixes the example in the above and the related Launchpad question (I guess, since there was no MWE therein ;-) ).

There are also several uses of aabb tuple stuff in /py/pack/_packPredicates.cpp which is untouched for now and requires more time to be looked at. It is not mandatory things are broken there.

[Robert Caulk (rcaulk)]

Thanks Jerome, I guess I did not understand the original proposal to only change the argument type of bodyNumInteractionsHistogram. Seems to be a better solution than changing each of the other functions I pointed out.

[Bruno Chareyre (bruno-chareyre)]

> several uses of aabb tuple stuff in /py/pack/_packPredicates.cpp which is untouched for now and requires more time

I feel uneasy about that. So, the list of potentially broken stuff after [1] includes some of Robert's list #1 plus /py/pack/_packPredicates? Is it right? Can there be other broken parts undiscovered yet?

If [1] was reverted, what would be the problem to solve, again? I suspect best solution is to fix that one problem with less side effects.

B

[1] https://github.com/yade/trunk/commit/1db13fb1183b9e294dc9761da76cfa4fc2791cc1

[Bruno Chareyre (bruno-chareyre)]

Incidentally, two students of mines got strange script failures this week and I realize only now that it was exactly this bug.
I don't want to open a new bug report but it seems "[1] is not well tested" could be one. Please Jerome consider, carefully, the option of reverting the initial commit. Changing so many unrelated functions just to implement a very specific feature is not reasonable. Or, please, make super-sure that it doesn't break anything before changing bottom layers of yade and pushing to trunk, not the case here unfortunately.
Bruno

[Jérôme Duriez (jduriez)]

I'll try to avoid exaggeration here. My commit [1] was about a Shop / utils function, maybe not exactly a bottom layer of yade (at least, not in my opinion).

As illustrated by the present bug and the launchpad question, [1] could be problematic in case of Python code (hardcoded in trunk or in users scripts) involving some other functions exposed through boost python with a specific type of arguments. It is true I did not foresee this problem in [1], in spite of the discussion [2] (see also therein for the motivations to commit [1])

As for #4 and Robert's list #1, I went through it and do not think there is now (was ?) any problem in this list due to commit [1], and in possible connection with the present bug.

I will look closely into py/pack/_packPredicates.

[1] https://github.com/yade/trunk/commit/1db13fb1183b9e294dc9761da76cfa4fc2791cc1
[2] https://<email address hidden>/msg13339.html, fourth item in particular...

[Bruno Chareyre (bruno-chareyre)]

If you have time now to make sure nothing is left behind broken it is perfectly fine.
Else a revert is not a curse and it takes much less time: no impact on you as you can still use the non-reverted version locally, no bad side effects on others.

[Jérôme Duriez (jduriez)]

For the record, I'm herein attaching a patch that would make aabbExtrema() back to a tuple, cancelling the effects of commits 1db13fb ( = [*] in the bug report) and 7c60d78 ( = more recent commit mentioned in #2)

As of now, I do not think this patch should be applied, see other comments regarding the (lack of) detected problems

[Jérôme Duriez (jduriez)]

The fact is py/pack/_packPredicates.cpp defines aabb() functions for predicates that still return tuples (not lists, as I proposed). I do not think this would necessarily conflict with the use of list in aabbExtrema(), but I still can not say I know all of _packPredicates.cpp

Then, in terms of a manual search for problems in pack-related scripts, I for instance tested:

    examples/gts-horse/gts-operators.py
    examples/gts-horse/gts-random-pack-obb.py
    examples/test/pack-cloud.py
    examples/test/pack-predicates.py
    examples/packs/packs.py
    examples/gts-horse/gts-horse.py
    examples/WireMatPM/wirepackings.py

which all worked well.

Other scripts showed a possibly more problematic behavior

    examples/gts-horse/gts-random-pack.py
=> pkg/common/Facet.cpp:26 postLoad: Facet has coincident vertices 2 (0 -0 -0) and 0 (0 -0 -0)!

    examples/polyhedra/horse.py
=> RuntimeWarning: No spheres are produced by regularHexa-function

    examples/stl-gts/gts-stl.py
=> ZeroDivisionError: float division by zero in regularHexa

I actually tested these scripts with three different versions of the executable:
- Yade 1.20.0 (yade package: aabbExtrema() is a tuple)
- current trunk after #2 ie commit 7c60d78 (aabbExtrema() is a list, after the previous "suspicious" commit)
- and commit 7c60d78 + the patch from #8 (aabbExtrema() is a tuple again).

The above possibly more problematic behaviors were always present in these three cases, suggesting they do not relate at all with the present "bug"... (hence my comment in #8)

Let me know in case you have further remarks.

[Bruno Chareyre (bruno-chareyre)]

Hi Jérôme, I reverted 7c60d78 and friend for you since I did not see reaction yet. We can't keep unsure code in daily builds, sorry.
Feel free to fix and recommit when everything is sorted out (if it is really worth it, I'm really not sure since the side effects are very intricate).
Bruno

[1] https://github.com/yade/trunk/commit/ac7647f55b739e324a56d1fcfd04ba63b63513de

[Bruno Chareyre (bruno-chareyre)]

Forgot to mention the related bug (fixed by the revert) https://bugs.launchpad.net/yade/+bug/1805416

[Bruno Chareyre (bruno-chareyre)]

After a bit more investigation the problem I see it that it is assumed everywhere that aabb's are tuples, and they are used for constructive solid geometry.
If predicate.aabb() is a tuple then aabbExtrema() must be a tuple as well, else the interface is heterogeneous and the extrema are incompatible with everything else (although it can still work in lucky cases because of intersections in the interfaces of tuples and lists).
Aabb type would then have to be changed in many places if we were changing it, and this is not limited to _packPredicates.cpp. See for instance cloudBestFitOBB() in _packObb.cpp. I still didn't spot all of them.

Although it was theoretically a good move to switch from tuple to wrapped vector<double>, it doesn't seem so wise now given the number of changes it implies.

Considering your very initial need which was IIRC to get the aabb of spheres I would suggest to either extract the values from the tuple, or to declare an additional function with same code as in 1db13fb11 (it could then be re-used inside aabbExtrema() to avoid duplication).

        Real inf=std::numeric_limits<Real>::infinity();
 Vector3r minimum(inf,inf,inf),maximum(-inf,-inf,-inf);
 FOREACH(const shared_ptr<Body>& b, *Omega::instance().getScene()->bodies){
  if(!b) continue;
  shared_ptr<Sphere> s=YADE_PTR_DYN_CAST<Sphere>(b->shape); if(!s) continue;
  Vector3r rrr(s->radius,s->radius,s->radius);
  minimum=minimum.cwiseMin(b->state->pos-(centers?Vector3r::Zero():rrr));
  maximum=maximum.cwiseMax(b->state->pos+(centers?Vector3r::Zero():rrr));
 }
 Vector3r dim=maximum-minimum;
 vector<Vector3r> ret;
 ret.push_back(minimum+.5*cutoff*dim);
 ret.push_back(maximum-.5*cutoff*dim);

Cheers

Bruno