← Back to team overview

yade-dev team mailing list archive

Re: [Bug 1804621] Re: bodyNumInteractionsHistogram broken

 

(moved to yade-dev)

Hi Jérôme, sorry but I can't agree with your last messages in the bug discussion.

> I do not think this patch should be applied
> regarding the (lack of) detected problems

Not sure why you write that there is no detected problem when we know _packPredicates.cpp is potentially broken.

The top priority is to keep all yade features functional always. You don't know how many people depend on _packPredicates.cpp (neither do I), and the example folder is a very partial view of what can be done. A general rule is: if a revision is found (or suspected) to break a feature then fix what was broken by the revision, or revert the revision. Either ways it must be done as soon as possible (a revert takes 30sec). Of course the revision can be later fixed and re-pushed.

I have personally no time to check details of this problem now. If it still needs thinking and discussion after three days then clearly reverting was the best solution in the first place; and still is.

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

So you seem still unsure. Which is understandable. Why you think it is important to share this suspicious commit with everyone through master branch is less clear. If you "cannot say", then revert. A revert takes 30sec and is not hurting anyone (100% sure thing), what is the rational to reject it?

We need to keep in mind that yade is deployed on clusters and that's nearly always based on latest version of source code. A bug like this means someone has to 1/ discover the problem, 2/ check yade forums and make the connection between his issue and a specific version, 3/ contact local IT support to request software update, 4/ IT then have to take action, 5/ and notify the user when the update is done, 6/ the user finally resumes working (that's probably the next day already even in the best cases). If you have never seen this cycle in action I can tell you more because that's exactly what I'm facing now (not only due to #1804621 "bodyNumInteractionsHistogram" bug, that's just one).

You are inserting an additional step in this workflow, between 2/ and 3/: wait days before updating the sources because some yade-devs prefer broken master over reverting a single commit. I'm sorry but it doesn't fit. People are waiting.
Please take action now.

Cheers
Bruno


On 11/26/18 5:54 PM, Jérôme Duriez 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.





Follow ups

References