yade-dev team mailing list archive

[Jan Stránský <honzik.stransky@xxxxxxxxx>]

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

> 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 ?
>
>
> 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
> [2] 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
>
> ------
> 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
> Post to     : yade-dev@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~yade-dev
> More help   : https://help.launchpad.net/ListHelp
>
>