Python wrapper last updates #357

tobadavid · 2017-01-10T21:20:29Z

This PR introduces the last updates for the Python wrapper :

Ability to run a SU2 computation that can interact with an external Python environment in order to exchange data (in both directions).
The code has been cleaned and comments were added.
The structure for an arbitrary MPI communicator (other than COMM_WORLD) to be passed to the Python wrapper is set but cannot be used at this time. COMM_WORLD is still used by default.
The Python wrapper is now compatible with Mac OS.

PR #356 has to be closed first. Then I will push a last commit after merging with the fixed develop.

…onstructor.

…definition of the SU2_Comm type in Python.

…_Comm datatype.

…f wrong use of the --parallel option when using the Py wrapper.

…efile. Bootstrap.

…ions.

…y in Python)

…mputations and FSI outputs

…ster.

…terface.

…tical).

tobadavid · 2017-01-11T11:02:38Z

Hi Tim,

Yes it would be great to have it like this indeed. Maybe we can keep it for a future PR ?

tobadavid · 2017-01-12T10:09:07Z

Thanks for merging Tim, but you did not solve correctly the conflict on parallel_regression.py. We do not test the external FSI coupling for now, so it has to be removed ;). Everything is up to date now.

talbring · 2017-01-12T10:11:58Z

Yep thanks! Didn't saw that when I merged it! Travis should pass now.

tobadavid · 2017-01-12T11:53:42Z

And Travis did pass. As for me, everything is ready now.

talbring

Just some additional comments.

talbring · 2017-01-12T16:16:13Z

SU2_CFD/src/driver_structure.cpp


 }

+unsigned long CDriver::GetNumberHaloVertices(unsigned short iMarker) {
+
+  unsigned long nHalovertices(0), iVertex, iPoint;


talbring · 2017-01-12T16:17:14Z

SU2_CFD/src/driver_structure.cpp

@@ -2781,27 +2854,48 @@ unsigned short CDriver::GetMovingMarker() {

 unsigned long CDriver::GetNumberVertices(unsigned short iMarker) {

-  unsigned long nFluidVertex;
+  unsigned long nVertices(0);


Although it is not wrong, but maybe use an assignment here, so that it is consistent with the rest of the code.

talbring · 2017-01-12T16:20:59Z

Common/include/datatypes/codi_reverse_structure.hpp

+
+#ifdef HAVE_MPI
+  typedef MPI_Comm SU2_Comm;
+#else


Then lets move at least these typedefs to the mpi structure for now.

…n driver_structure.

tobadavid · 2017-01-12T17:15:58Z

I made the changes. Waiting for Travis.

talbring · 2017-01-12T17:25:25Z

Common/include/datatypes/codi_forward_structure.hpp

@@ -34,5 +34,9 @@

 #include "codi.hpp"

+#ifdef HAVE_MPI


You probably just forgot to remove these.

When you look at the file everything seems to be ok, isn't it ?

Oh no, I see...let me change this !

talbring

Thanks David! 👍

economon

Getting close, David!

Just a few typos and comments here in the review. I guess the biggest thing is finalizing the name for the wrapped module...

economon · 2017-01-13T03:57:14Z

SU2_CFD/include/driver_structure.hpp

@@ -273,30 +271,204 @@ class CDriver {
   */
  virtual void SetInitialMesh() { };

-  /*--- External communication layer ---*/
-
+  /*!


Thanks for commenting these..

economon · 2017-01-13T04:01:02Z

SU2_CFD/src/driver_structure.cpp

+
+  unsigned short val_iZone = ZONE_0;
+  unsigned short FinestMesh = config_container[val_iZone]->GetFinestMesh();
+  su2double CMy, RefDensity, RefAreaCoeff, RefLengthCoeff, RefVel2(0.0), factor;


Check RefVel2 initialization

economon · 2017-01-13T04:01:21Z

SU2_CFD/src/driver_structure.cpp

+
+    unsigned short val_iZone = ZONE_0;
+    unsigned short FinestMesh = config_container[val_iZone]->GetFinestMesh();
+    su2double CDrag(0.0);


CDrag initialization

economon · 2017-01-13T04:01:39Z

SU2_CFD/src/driver_structure.cpp

+
+    unsigned short val_iZone = ZONE_0;
+    unsigned short FinestMesh = config_container[val_iZone]->GetFinestMesh();
+    su2double CLift(0.0);


CLift initialization

economon · 2017-01-13T04:02:50Z

SU2_CFD/src/driver_structure.cpp

-    //if(geometry_container[ZONE_0][MESH_0]->node[iPoint]->GetDomain()) partFluidSurfaceLoads[iVertex][0] = GlobalIndex;
-    //else partFluidSurfaceLoads[iVertex][0] = -1.0;
-    if(geometry_container[ZONE_0][MESH_0]->node[iPoint]->GetDomain()) {
+  /*--- It necessary to distinguish the halo nodes from the others, since they introduice non physical forces. ---*/


typo in "introduce"

economon · 2017-01-13T04:10:25Z

SU2_PY/SU2_CFD.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python

-## \file test_pyWrapper.py
-#  \brief Python script to launch SU2_CFD through the Python Wrapper
+## \file SU2_CFD.py


Thanks for switching the name!

economon · 2017-01-13T04:10:57Z

SU2_PY/SU2_CFD.py

-    SU2Driver = SU2Solver.CHBDriver(options.filename, options.nZone, options.nDim);
-  elif (options.nZone == 2) and (options.fsi):
-    SU2Driver = SU2Solver.CFSIDriver(options.filename, options.nZone, options.nDim);
+  if options.with_MPI == True:


This is a nice addition so that folks can run in serial through Python.

economon · 2017-01-13T04:14:10Z

SU2_PY/pySU2/pySU2.i


 %feature("autodoc","1");

 %module(docstring=
-"'SU2Solver' module",
+"'WrapSU2' module",


The best name choice is still unclear for me.. ideally it would just be SU2, but the old Python framework already uses this. Perhaps "pySU2" instead? Any suggestions?

tobadavid · 2017-01-13T09:58:39Z

Thanks Tom !

I made the changes (typo and initializations).

I would not choose just "SU2" for the Py wrapper name, since there already is a SU2 python folder. By the way this also contains some python driving functions and this might be confusing. The true python wrapper is what you get when you directly interface C++ functionalities with python through SWIG.

The previous name of the wrapper was SU2Solver but once again I would avoid it since the true goal of the wrapper is to interface functionalities that could be much deeper than just the solver level. The python wrapper is not just a solver.

So to avoid confusion I chose the name WrapSU2. pySU2 would do the job too and this is consistent with the pySU2 folder (used to compile the wrapper). SU2Wrapper ? PyWrapSU2 ? Just keep in mind that "SU2" should be part of the name, imagine you have to import two wrapped modules (from different codes) in the same python script I would prefer having :

import pySU2 # wrapper of SU2
import pyOtherCode # wrapper of another code

than having :

import pyWrap # wrapper of SU2 but this is unclear
import theWrapper # wrapper of an other code but this is unclear

talbring · 2017-01-13T14:10:57Z

In my opinion pySU2 would be a good name.

tobadavid · 2017-01-13T15:17:31Z

I agree, let's wait for other suggestions if any.

economon · 2017-01-13T19:27:05Z

I'm fine with pySU2 as well, but yes, let's hear any other ideas.

arubino · 2017-01-14T16:01:36Z

pySU2 looks good to me!

petebachant · 2017-01-15T15:53:07Z

If you guys are looking to follow PEP8, which is always nice, the package name would be all lowercase, i.e., pysu2.

tobadavid · 2017-01-15T17:05:22Z

Thanks ! So do we go for pysu2 as the Py module and _pysu2 as the extended C++ (.so lib) ?

economon · 2017-01-18T21:13:31Z

Thanks for the input, all, and for the final adjustment, David. Time to merge this in!

tobadavid added 30 commits August 16, 2016 15:38

Introduce a new SU2_Comm conditional datatype.

c1b4218

Introduce a SU2_Comm communicator into the configuration container.

c908e4d

Introduce a SU2_Comm communicator as a new argument for the CDriver c…

1f04d77

…onstructor.

Introduce a new --parallel option in order to handle the conditional …

c3c4a79

…definition of the SU2_Comm type in Python.

Improve the compilation of the Python wrapper with mpi4py and the SU2…

820938e

…_Comm datatype.

Improve the management of serial vs parallel run.

9aacbb8

Switch travis config in order to run local regressions.

4f5d31c

Rename the CFD Python wrapper launching script.

c79ed81

Extend the MPI communicator framework to the other SU2 modules.

8b71aa6

Print clear error messages and cleanly exit the computation in case o…

09781e8

…f wrong use of the --parallel option when using the Py wrapper.

Merge branch 'develop' into feature_pyWrapper

b9ce7a8

Merge with the fixed develop. Solve small conflicts for the pySU2 Mak…

b48991d

…efile. Bootstrap.

Fix a compilation error with the typedef SU2_Comm for build with AD.

cb20665

Merge branch 'develop' into feature_pyWrapper

7a00902

Update files headers

66671c0

Fix a bug when make install is direclty used for the Py wrapper.

b755ec9

Add a function to identify the halo nodes for external FSI communicat…

f4e0950

…ions.

New parallel implementation of the FSI interface using PETSc (petsc4p…

dcd4da3

…y in Python)

Add the installation of petsc4py to travis.

7823d65

Try to fix a bug with mpif90 on travis.

17d967d

Try to fix a bug with mpif90 on travis.

890e7e0

Improve and fix a small bug in relaxation step, improve steady FSI co…

22f5070

…mputations and FSI outputs

Temporarily turn off Travis tests.

82cb637

Upgrade the communication layer of the PitchPlungeAirfoilStructuralTe…

261714a

…ster.

Add NN, RBF and TPS interpolation for non-matching fluid-structure in…

f7d40c7

…terface.

Add new options for non-matching meshes interpolation.

25164d6

Add some external communications to the driver.

2503598

Change the name of the SU2 Python wrapper.

b142e5c

Merge develop into feature_pyWrapper and solve conflicts (nothing cri…

5aa8575

…tical).

Adapt to the new name of the Python wrapper module.

ccc3f8d

tobadavid and others added 6 commits January 11, 2017 13:09

Rename the Py wrapped version of SU2_CFD.

3e4afbc

Ensure the Py wrapper will compile without TecIO support.

990933e

Merge branch 'develop' into feature_pyWrapper

267893c

Merge with develop (one small conflict fixed).

55c6b3a

Merge branch 'develop' into feature_pyWrapper

edd9150

Remove FSI external coupling reg test again.

bf842e3

talbring reviewed Jan 12, 2017

View reviewed changes

Move SU2_Comm typedef into mpi_structure and change two assignments i…

63e3517

…n driver_structure.

talbring reviewed Jan 12, 2017

View reviewed changes

tobadavid added 2 commits January 12, 2017 22:29

Remove include of mpi in some datatypes structures.

6dc624c

Merge branch 'develop' into feature_pyWrapper

5b9c0d7

talbring approved these changes Jan 12, 2017

View reviewed changes

economon reviewed Jan 13, 2017

View reviewed changes

Fix typo and some initializations.

7417236

Merge branch 'develop' into feature_pyWrapper

a76a25b

Rename the Py wrapper module.

400a38a

economon merged commit 02627aa into develop Jan 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python wrapper last updates #357

Python wrapper last updates #357

tobadavid commented Jan 10, 2017

tobadavid commented Jan 11, 2017

tobadavid commented Jan 12, 2017

talbring commented Jan 12, 2017

tobadavid commented Jan 12, 2017

talbring left a comment

talbring Jan 12, 2017

talbring Jan 12, 2017

talbring Jan 12, 2017

tobadavid commented Jan 12, 2017

talbring Jan 12, 2017

tobadavid Jan 12, 2017

tobadavid Jan 12, 2017

talbring left a comment

economon left a comment

economon Jan 13, 2017

economon Jan 13, 2017

economon Jan 13, 2017

economon Jan 13, 2017

economon Jan 13, 2017

economon Jan 13, 2017

economon Jan 13, 2017

economon Jan 13, 2017

tobadavid commented Jan 13, 2017

talbring commented Jan 13, 2017

tobadavid commented Jan 13, 2017

economon commented Jan 13, 2017

arubino commented Jan 14, 2017

petebachant commented Jan 15, 2017

tobadavid commented Jan 15, 2017

economon commented Jan 18, 2017

Python wrapper last updates #357

Python wrapper last updates #357

Conversation

tobadavid commented Jan 10, 2017

tobadavid commented Jan 11, 2017

tobadavid commented Jan 12, 2017

talbring commented Jan 12, 2017

tobadavid commented Jan 12, 2017

talbring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobadavid commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talbring left a comment

Choose a reason for hiding this comment

economon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobadavid commented Jan 13, 2017

talbring commented Jan 13, 2017

tobadavid commented Jan 13, 2017

economon commented Jan 13, 2017

arubino commented Jan 14, 2017

petebachant commented Jan 15, 2017

tobadavid commented Jan 15, 2017

economon commented Jan 18, 2017