Re: [Yade-dev] Why _utils.hpp and Shop.hpp ? Shop::getPorosity vs Shop__getPorosity

2018-07-17 Thread Bruno Chareyre 







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


Agreed.
The idea of not calling Omega::instance().getScene() all the time is 
sound since it has a cpu cost.
Arguably this cost is negligible compared to effectively calculating 
porosity but it is not always the case in all functions, it is thus a 
good rule of thumb to pass it as argument in c++ algorithms.

Bruno




___
Mailing list: https://launchpad.net/~yade-dev
Post to : yade-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~yade-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Yade-dev] Why _utils.hpp and Shop.hpp ? Shop::getPorosity vs Shop__getPorosity

2018-07-16 Thread Jerome Duriez 

Thanks for the 1st opinion, Jan.

To be clear, I do not want to propose any changes, it's just I'm writing 
these days C++ functions with Python exposure, and I try to understand 
the rationale (if any) behind this architecture before I reproduce it...


Jérôme

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

On 16/07/2018 14:14, Jan Stránský wrote:

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


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@lists.launchpad.net

Unsubscribe : https://launchpad.net/~yade-dev

More help   : https://help.launchpad.net/ListHelp





___
Mailing list: https://launchpad.net/~yade-dev
Post to : yade-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~yade-dev
More help   : https://help.launchpad.net/ListHelp



___
Mailing list: https://launchpad.net/~yade-dev
Post to : yade-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~yade-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Yade-dev] Why _utils.hpp and Shop.hpp ? Shop::getPorosity vs Shop__getPorosity

2018-07-16 Thread Jan Stránský 
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 :

> 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@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~yade-dev
> More help   : https://help.launchpad.net/ListHelp
>
>
___
Mailing list: https://launchpad.net/~yade-dev
Post to : yade-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~yade-dev
More help   : https://help.launchpad.net/ListHelp

[Yade-dev] Why _utils.hpp and Shop.hpp ? Shop::getPorosity vs Shop__getPorosity

2018-07-16 Thread Jerome Duriez 

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@lists.launchpad.net
Unsubscribe : https://launchpad.net/~yade-dev
More help   : https://help.launchpad.net/ListHelp