yade-dev team mailing list archive

[Bruno Chareyre <bruno.chareyre@xxxxxxxxxxxxxxx>]

(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.