bodyNumInteractionsHistogram broken

Bug #1804621 reported by Jérôme Duriez on 2018-11-22
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Yade
Wishlist
Bruno Chareyre

Bug Description

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) wrote :

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

Changed in yade:
assignee: nobody → Jérôme Duriez (jduriez)
Jérôme Duriez (jduriez) wrote :

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) wrote :

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) wrote :

> 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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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.

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

Changed in yade:
importance: Undecided → Wishlist
status: New → Triaged
assignee: Jérôme Duriez (jduriez) → Bruno Chareyre (bruno-chareyre)
status: Triaged → Fix Released

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

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

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Related questions