Hi Jerome,
below please find my opinion. If the approach proposed by you works
and does not break existing code (or it is not easy to fix it), I
would have nothing against the refactoring.
What is the reason for having (for instance) Shop__getPorosity()
in py/_utils.hpp/cpp and Shop::getPorosity() in pkg/dem/Shop.hpp /
Shop_01.cpp ?
In **my opinion**:
1) Shop functions should be C++ only, not returning or accepting
boost::python stuff. The python stuff (like converting std::vector to
python tuple/list or modifying input/output like [1]) is (should be)
the reason for py/_utils existence.
2) In the same way, there should not be non-python function in
py/_utils (should be moved to Shop).
Not 1) nor 2) is the case now..
Maybe the scene as argument is left from the past and it is safe to
remove it?
I wanted to write that another reason could be compatibility, because
come functions are named differently in C++and python (e.g.
bodyStressTensor and getStressLWForEachBody), but this is irrelevant
since you can have any name for python functions (put here for
backward reference if anybody has the same idea :-)
having py/_utils.*pp in addition to pkg/dem/Shop*
Both following comments are extensions of my first paragraph
There are some "python wrapping", see e.g. [1], where the inputs and
return types are a bit modified to be more user friendly (you don't
need to create a Material first, pass it to function and dig data from
it, a plain dict is returned).
But more importantly, to be able to have these Shop functions in
Python, you need some boost::python tricks, which IMO should be placed
in a separate file, so py/_utils.*pp would stay anyway (although it
could be cleaner).
cheers
Jan
[1] https://github.com/yade/trunk/blob/master/py/_utils.cpp#L111
2018-07-16 11:21 GMT+02:00 Jerome Duriez <jerome.duriez@xxxxxxxxx
<mailto:jerome.duriez@xxxxxxxxx>>:
Hi,
What is the reason for having (for instance) Shop__getPorosity()
in py/_utils.hpp/cpp and Shop::getPorosity() in pkg/dem/Shop.hpp /
Shop_01.cpp ?
I can see the latter has "scene" as an argument [1] in addition to
"volume" (contrary to the former [2]), but probably this could be
removed from the definition, always using
Omega::instance().getScene() in the implementation [3] ? So, I'm
not sure this is the reason
Is this for Python wrapping ? Could we not use Shop::getPorosity
(once we removed the scene argument) at
https://github.com/yade/trunk/blob/master/py/_utils.cpp#L463
<https://github.com/yade/trunk/blob/master/py/_utils.cpp#L463> ?
The same question goes for many other functions, I guess, and the
general philosophy behind having py/_utils.*pp in addition to
pkg/dem/Shop*
Thank you,
Jérôme
[1] https://github.com/yade/trunk/blob/master/pkg/dem/Shop.hpp#L56
<https://github.com/yade/trunk/blob/master/pkg/dem/Shop.hpp#L56>
[2] https://github.com/yade/trunk/blob/master/py/_utils.hpp#L123
<https://github.com/yade/trunk/blob/master/py/_utils.hpp#L123>
[3] just modifying
https://github.com/yade/trunk/blob/master/pkg/dem/Shop_01.cpp#L300
<https://github.com/yade/trunk/blob/master/pkg/dem/Shop_01.cpp#L300>
------
Chargé de Recherche / Research Associate
Irstea, RECOVER
3275 route de Cezanne – CS 40061 13182 Aix-en-Provence Cedex 5 FRANCE
+33 (0)4 42 66 99 21
_______________________________________________
Mailing list: https://launchpad.net/~yade-dev
<https://launchpad.net/%7Eyade-dev>
Post to : yade-dev@xxxxxxxxxxxxxxxxxxx
<mailto:yade-dev@xxxxxxxxxxxxxxxxxxx>
Unsubscribe : https://launchpad.net/~yade-dev
<https://launchpad.net/%7Eyade-dev>
More help : https://help.launchpad.net/ListHelp
<https://help.launchpad.net/ListHelp>
_______________________________________________
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
