Kratos: Naming conventions in Python files

[jcotela]

As noted in the discussion of #30 we have no unified criteria on naming Python methods (and in Python in general). Summarizing the discussion, PEP 8 provides some recommended best practices, which I think would be good to follow.

The immediate issue in #30 was if we want to use the single underscore _method(), which just hides the method, or the double leading underscore __method(), which also mangles the name (so it can't be easily used on derived classes), but I think a wider discussion is in order.

I would also like to raise the point of defining the __all__ variable in our modules, which is something we have never done and would be very convenient when doing from ... import * statements.

@KratosMultiphysics/technical-committee what is the 'official' view on this?

[marandra]

+1 to following PEP8, conventions and code formatting.

-10 to from ... import *, however. Besides its potential problems with name collisions, ... import * makes it more difficult to follow and debug code. Particularly for beginners, given that it is coded in this way in most test-example MainKratos.py and templates for GiD problem types.

[jcotela]

It is not so much that we want absolute imports, but that they are already all over the place, so we should be careful not to make matters worse. In any case, I am in favor of some limited uses of absolute imports. Most notably, using variable names as relative imports (KratosMultiphysics.VELOCITY) is unnecessarily cumbersome, even when using import as to introduce shorter module names.

[pooyan-dadvand]

+1 for PEP8

And +1 for (KratosMultiphysics.VELOCITY) is unnecessarily cumbersome

[maceligueta]

¿Is there a possibility to import Kratos without 'import *' but then add a line importing the variables like 'from KratosMultiphysics.Variables import *' ?

[marandra]

EDIT: @maceligueta I was replying to the previous posts

I was actually thinking of variables mainly. I write all the Python code in our app naming them explicitly, as sometimes there might be conflicts (e.g. import KratosMultiphysics as km, import KratosMultiphysics.MyApplication as kma, km.LAGRANGIAN_MULTIPLIER, kma.LAGRANGIAN_MULTIPLIER), particularly because I am using and redefining other Apps components, and name conflicts occur.

Take my opinion from the beginner point of view of course, but the first think I needed to do to make sense of Kratos when I started, was to re-write Python code to add the module name components belong to (e.g. km.VELOCITY). In my opinon (at the time, and today) the code was much more clean, clear and robust. I didn't think of it as cumbersome (the key here is as), quite the opposite I think it totally paid-off.

Today I can find my way in code with import *, but that is because I have more experience, not because the code is cleaner neither more robust. For me now is easier to understand when it is causing problems and how to fix it (expliciting the module), but this is still not the point.

I totally it may require effort the remove the from ... import * making sure we don't introduce bugs, but I will approach it as first stating this convention for any new code, and secondly modifying the templates from GiD problem type.

[jcotela]

While I completely agree with you regarding modules, I have quite the opposite opinion for variables. For me, which application defines what variable is an implementation detail. Typically I will add a variable in my application to perform some task and maybe later move the variable to the core so that a colleague can use it. When this happens, I don't want to change all my python scripts. I would also prefer to not pass this burden to the end user. I may know that VORTICITY is a core variable and not a FluidDynamicsApplication variable even though it is a typically CFD quantity because I defined it, but I would not want to require my users to know it (by the way, I had to check it).

A related problem (which neither alternative solves completely) is that we don't want to make it easy to enable users to define two variables with the same name in different applications. How do you know which variable are you using when you write LAGRANGIAN_MULTIPLIER in your C++ code? I would assume this depends on which includes you have in your file (and in which order?).

My prefered usage would be something like @maceligueta suggests, but we have to see how to realize it technically.

[RiccardoRossi]

actually i completely agree with Marcelo (@marandra)

cheers
Riccardo

On Fri, Feb 17, 2017 at 11:02 AM, marandra [e-mail] wrote:

I was actually thinking of variables mainly. I write all the Python code
in our app naming them explicitly, as sometimes there might be conflicts
(e.g. import KratosMultiphysics as km, import KratosMultiphysics.MyApplication
as kma, km.LAGRANGIAN_MULTIPLIER, kma.LAGRANGIAN_MULTIPLIER),
particularly because I am using and redefining other Apps components, and
name conflicts occur.

Take my opinion from the beginner point of view of course, but the first
think I needed to do to make sense of Kratos when I started, was to
re-write Python code to add the module name components belong to (e.g.
km.VELOCITY). In my opinon (at the time, and today) the code was much
more clean, clear and robust. I didn't think of it as cumbersome (the key
here is as), quite the opposite I think it totally paid-off.

Today I can find my way in code with import *, but that is because I have
more experience, not because the code is cleaner neither more robust. For
me now is easier to understand when it is causing problems and how to fix
it (expliciting the module), but this is still not the point.

I totally it may require effort the remove the from ... import * making
sure we don't introduce bugs, but I will approach it as first stating this
convention for any new code, and secondly modifying the templates from GiD
problem type.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/KratosMultiphysics/Kratos/issues/42#issuecomment-280608322,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHr7ETCWUYmPrUYmdKmtR807EJHe7OVeks5rdXBAgaJpZM4MDF0z
.

--

Riccardo Rossi

PhD, Civil Engineer

member of the Kratos Team: www.cimne.com/kratos

Tenure Track Lecturer at Universitat Politècnica de Catalunya,
BarcelonaTech (UPC)

Full Research Professor at International Center for Numerical Methods in
Engineering (CIMNE)

C/ Gran Capità, s/n, Campus Nord UPC, Ed. B0, Despatx 102

(please deliver post to the porters of building C1)

08034 – Barcelona – Spain – www.cimne.com -

T.([phone] skype: rougered4

http://www.cimne.com/

https://www.facebook.com/cimne http://blog.cimne.com/
http://vimeo.com/cimne http://www.youtube.com/user/CIMNEvideos
http://www.linkedin.com/company/cimne https://twitter.com/cimne

Les dades personals contingudes en aquest missatge són tractades amb la
finalitat de mantenir el contacte professional entre CIMNE i voste. Podra
exercir els drets d'accés, rectificació, cancel·lació i oposició,
dirigint-se a [email protected]. La utilització de la seva adreça de
correu electronic per part de CIMNE queda subjecte a les disposicions de la
Llei 34/2002, de Serveis de la Societat de la Informació i el Comerç
Electronic.

Imprimiu aquest missatge, només si és estrictament necessari.
http://www.cimne.com/

[rubenzorrilla]

I agree with @marandra and @RiccardoRossi. It is quite useful, specially for beginners, to know from where are you importing a module. Besides lots of conflicts can be avoided (imagine combined solvers such as FSI).

However as @jcotela points this becomes in a mess when a variable is moved from an app to the core. I do not know any technical solution to this issue, but if there is no solution or if it is to complex at this point, we have to think if this variable migration from app to core is common. However, this allows the user to repeat variable names...

[pooyan-dadvand]

I agree with @jcotela

Exposing the implementation details to python scripts is not the best idea and pushes lot of maintenance overhead.

And to know where is defined one can use the __module__ method:

VELOCITY.__module__

which gives:

'Kratos'

as result which is the Kratos.so where defined.

[marandra]

@maceligueta I imagine one can create something like a submodule Variables and have something like this:
import KratosMultiphysics as km
from KratosMultiphysics.Variables import *
import kratosMultiphysics.SolidMechanicsApplicarion as ksm
from kratosMultiphysics.SolidMechanicsApplicarion.Variabled import *
but I am not sure if we would be solving the real problem here.

About issue of whether to allow duplicated names of variables or not. What about the other components beside variables? In the negative case, how do we enforce it? What are the problems involved here? What are the gains? I think all the above is for other issue, doesn't change the conclusions of this "import star" issue

From my point of view, what matters is that Python is usually used in high level applications, where we mix and match core and applications. The code should be very clear and unambiguous. I don't think we are helping the user by hiding the origin of the components.

Also, if we don't use import *, the user only needs to know Python to follow the code. If we do use it, the user need to know Python and Kratos to follow it (i.e. to guess that he/she can make VELOCITY.__module__)

[jcotela]

Duplicate variables are definitely not recommended. We have had bad experiences with this in the past, especially on the C++ parts (where we have nothing like Python modules) and, if my understanding is correct, they are actually forbidden, although we have no mechanism to enforce it. Also, @pooyan-dadvand please correct me if I'm wrong, but defining duplicate variables in different applications will be explicitly blocked in the future if we go on with the current plans for the core.

Returning to the issue at hand, I'm reluctant to enforce the relative import of variable names because the variables themselves are perfectly equivalent. They are designed to be used interchangeably, so that I can store and access data on nodes (or elements or...) by using a universal key, that is my variable. In this sense, they are a bit weird: different variables are not used as different objects, but like different indices to access the same array or different strings to query the same dictionary. In my view, qualifying them with a module prefix provides little meaningful information (variables defined in different applications can be used interchangeably) and a lot of bookkeeping (which application defines which variable).

By the way, the __module__ feature is a Python internal, it has nothing to do with Kratos.

[RiccardoRossi]

Hi All,
i am writing here just to report the points of agreement between me and @jcotela :-) (please Jordi, correct me if my resume is incorrect)

let's consider we have a directory

CFD/python/scripts

which contains 2 files, each implementing a given function f
a.py
b.py

the agreement is that in order to import a we should be able to do:

 from CFD import *
 a.f()
 b.f()

on the other hand we should have

 import CFD
 CFD.a.f()
 CFD.b.f()

a different issue, on which we have not yet reached an agreement is what happens if we have for example a "Process" implemented either in python or in cpp

let's consider here two applications:

• CFD defining pyProc (implemented in CFD.pyProcFile) and cppProc
• SM defining pyProc (implemented in SM.pyProcFile) and cppProc

in order to import the python processes we would need to do:

import CFD
new_py_proc = CFD.pyProcFile.pyProc(....)  //let's assume here the parameters passed to the constructor are always the same
new_cpp_proc = CFD.cppProc(...)

the are a few options here
1 - implementing a function "Factory" in pyProcFile and give the information of the filename (class name is not needed here). This is the mechanism used right now
2 - creating a registration mechanism, in which one defines a function say:

   kernel.RegisterPythonProcess("name", "filename", application)

we would also have a similar function in c++ registering all of the processes say:. Here the discussion

   kernel.RegisterProcess("name", application)

an open point is also whether we shall maintain manually the list of processes or if we should generate it automatically, as well as where such a list should go.
3 - creating a "factory class" like the "linear solver factory" which hides from the user the nitty-gritty details

each option has advantage and disadvantages.
Just to tell which is my personal preference, I am for option 1. Second option (again for me) is 2, but in this case I would prefer automatic generation to mantaining the class manually.

A point of discussion is also whether we shall allow two classes with the same name to be defined in CFD and SM.

[jcotela]

Regarding absolute and relative imports, note that this is different than what is currently implemented. Going with the example given by @RiccardoRossi, if we have a.py (defining function f()) and b.py (again defining f()) in CFD (an application), today we do:

from CFD import *
import a
a.f()
import b
b.f()

This is opposed to what happens with a g() function defined in source file c.cpp and exposed to Python:

from CFD import *
g()

One of the things I would like to change going forward is to ensure that Python and C++ classes and functions are used in the same way, as much as possible.

[pooyan-dadvand]

+1 for @jcotela

[loumalouomega]

Maybe we should add this to the https://github.com/KratosMultiphysics/Kratos/wiki/Style-Guide

[RiccardoRossi]

Can this be closed? Ideally updating the wiki...

[jcotela]

I don't think we ever reached a decision :(

[philbucher]

guys can we bring this in the focus again?

At least having a style guide how (class) member functions and member variables would be nice. And also for function arguments and local variables

We discuss this here on a regular basis but without conclusion. :D
@dbaumgaertner @armingeiser @AndreasWinterstein

[philbucher]

for classes it would be good to have definitions for (examples are from structural_mechanics_solver.py in StructuralMechanics)

• Public (e.g. AddVariables())
• Protected (e.g. get_solution_scheme())
• Private (e.g. _add_dynamic_dofs())
  I know the concept of private/public doesn't exist in Python but I think it is still very helpful to know which functions are intended to be used in which way

[jcotela]

@philbucher for python, there is a kind of informal convention (that we have never really followed in Kratos) here.

• "Internal" methods start with an underscore _method(). This I understand as similar to a protected.
• "Hidden" methods start with two underscores __method(). This provides name mangling and, for example, this method won't be listed when you try dir(my_class). I understand this as the closest we'll get to private in C++.

[RiccardoRossi]

I would add to Jordi's comment a suggestion: i THINK it is possible to force implementing a method by definiing it as "@abstractmethod"

[philbucher]

thx @jcotela
And public functions? We seem to be using them in UpperCamelCase, would that be ok?

[jcotela]

@philbucher for me yes. For what is worth, the official Python recommendation is snake_case (see PEP8), but adopting this would be a huge change for us.

[philbucher]

@KratosMultiphysics/technical-committee I suggest to add the following to the style guide:
Classes

• Class names: UpperCamelCase
• "Public" functions: UpperCamelCase
• "Protected" Functions: _lower_case_with_one_underscore
• "Private" Functions: __lower_case_with_two_underscores
• Member Variable names: lower_case_with_underscores

Function arguments

• lower_case_with_underscores

Local Variables

• lower_case_with_underscores (same as for cpp)

This would follow (mostly) PEP8
I can add a first version to the style guide, then we have sth to improve on

[dbaumgaertner]

I saw that still no style guide for python is in the wiki. So following the above discussion I added the suggestions summarized by @philbucher. Please feel free to edit as necessary.

[dbaumgaertner]

In case things are fine, we might probably close this branch.

[philbucher]

@dbaumgaertner thx for the reminder.
I only saw now that several ppl replied with thumbs up, but I wasn't notified by this.
I will add it to the style guide in the next days and close this issue then.
Until then you can refer to my suggestions above for the namings

[dbaumgaertner]

@philbucher Already done, so I will close this issue.

[philbucher]

Thx @dbaumgaertner !

[dbaumgaertner]

After changing some of our code maybe a suggestion: Why not making it easier to memorize this in the sense that arguments & variables are generally written in lower_camel_case, whereas class names & functions are generally written in UpperCamelCase. The latter could still be distinguished by one or two leading underscores for "public" and "private" functions, but without the somewhat unlogic change to lower_camel_case.

[philbucher]

I.e. for classes:

• "Public" functions: UpperCamelCase
• "Protected" Functions: _UpperCamelCaseWithOneLeadingUnderscore
• "Private" Functions: __UpperCamelCaseWithTwoLeadingUnderscores

I also would prefer this bcs this way it is way easier to distinguish functions from variables

[pooyan-dadvand]

This would be more aligned with our c++ style!

[philbucher]

see the style guide for python
thx @dbaumgaertner