yade-dev team mailing list archive

[Bruno Chareyre <bruno.chareyre@xxxxxxxxxxxxxxx>]

Here is the solution, maybe. It still needs confirmation:

pair<Vector3r,Vector3r> aabbExtrema(...);
py::tuple aabbExtrema_py(...);
py::def("aabbExtrema",Shop::aabbExtrema_py,...)

Bruno

On 11/26/18 7:54 PM, Bruno Chareyre wrote:
> (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.
>
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~yade-dev
> Post to     : yade-dev@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~yade-dev
> More help   : https://help.launchpad.net/ListHelp