MPI Point-to-Point Refactoring + New Periodic BC Implementation #652

economon · 2019-02-15T20:58:35Z

Proposed Changes

Give a brief overview of your contribution here in a few sentences.

This is one for v7 and needs some testing. This PR contains a complete refactoring of the point-to-point MPI infrastructure and an entirely new implementation of the periodic BC for the FV solvers.

For the MPI refactoring:

All calls are abstracted to parent classes (geometry, solver, matrix), meaning we will remove all Set_MPI_* routines.
Point-to-point comms are all non-blocking calls that have been separated into InitiateComms() & CompleteComms(), so that, in theory, they can be overlapped with computation where possible. It would also be possible now to group the comms into larger messages easily. These optimizations are a next step (not for this PR).
Persistent communication schedules and memory are used throughout the calculation.
Results are the same, this is just a refactoring to improve performance, scalability, etc. while making the code much simpler and separating the periodic part.

For the periodic BC:

No longer need a preprocessing with SU2_MSH - use the original grid and just call SU2_CFD.
Adjacent periodic faces are now possible, e.g., triply-periodic cube.
Concept is now based on completing the residuals from partial control volumes on either side of a periodic face instead of creating extra halo cells.
All periodic comms have been separated from the MPI point-to-point comms and are performed first for consistency.

In a follow-up PR, I will remove many lines of code that are no longer needed (all Set_MPI_* routines, many outdated geometry routines, etc.)

Related Work

Resolve any issues (bug fix or feature request), note any related PRs, or mention interactions with the work of others, if any.

I could use some help with testing this one, in particular the rotational periodicity, which might still need some changes. Unfortunately, I do not have the original meshes for the periodic cases in the repo that are failing (other than Poiseuille)..

@salvovitale @LaSerpe @arubino: do you have the original grids for the periodic cases before adding the halos with SU2_MSH? I would like to make sure that everything is working ok for all previous capability.

All periodic meshes in the regressions have been updated for the new periodic BC, and a python script has been added to remove halo cells from old meshes that were preprocessed with SU2_MSH for the previous periodic BC implementation.

Resolves #550
Resolves #431
Resolves #416
Resolves #252

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags).
My contribution is commented and consistent with SU2 style.
I have added a test case that demonstrates my contribution, if necessary.

…be generalized to all solvers and parallelization should be improved.

…sors, and eigenvalues. Both Euler and N-S operational. Fixed multiple bugs.

economon · 2019-04-19T00:23:30Z

This PR is ready from my point of view. All periodic / turbomachinery meshes and regressions have been updated (thanks @salvovitale), and I can confirm they converge with the new periodic implementation. Only a few other regression tests needed to be updated due to the refactoring of the MPI, but they were very minor.

If the tests pass, then I recommend we merge very soon. Any final comments or reviews are most welcome.

EduardoMolina · 2019-04-20T03:21:37Z

Hi Tom,

Thanks for this great PR. I have two notes:
a) I have one grid (flow around a high lift airfoil) where the periodic boundaries are not placed entirely on fix plane, like y=0.0, but on a very small value and fluctuates around, i.e. y~1.e-5. Now, many bad match points are found in the last commit of this PR. Previously, I was able to run the same mesh in a different commit. Do you change the tolerance of the search algorithm? Or I need to make sure now that all the periodic BC nodes must be placed on a fix plane.
b) I would like to share the results of the Taylor Green Vortex with the community. Indeed, the results are good and they are comparable with the reference DNS solution.

Thank you one more time.

Best,

Eduardo

SU2_CFD/src/driver_structure.cpp

rsanfer

LGTM, @economon, thanks so much for this great effort. Really looking forward to that follow-up PR that will remove all the Set_MPI_*** routines.

Practical aspect: @economon @pcarruscag let me know how you want to handle merging this and #653, as they are both ready to go in but there will be some small conflicts.

economon · 2019-04-23T18:55:50Z

@EduardoMolina : Nice results! Thanks for the comments. You are seeing more warnings bc I did in fact change the tolerance that controls the warning. But, note that this tolerance only controls when to print the warnings, so the matches should have still be the same (nearest neighbor after transformation).

The reason is that the new implementation assumes a 1-to-1 matching of the nodes and surface elements on either side of the periodic face. To be consistent, the points/faces on either side should match exactly so that they can be summed into a complete dual control volume (this assumption is made in the implementation and the partial residuals are summed as such).

Nice catch on the fem_solver check. I will add that to the conditional.

Thanks for the review @rsanfer. I will submit the final version shortly and if the tests pass, I would propose we merge this one first, if alright with @pcarruscag (the reason being that I would likely need your help to update the implementation for templating in the end anyway :) ).

…res with the DG solver.

economon · 2019-04-24T01:26:23Z

Thanks for the input, everyone. Time to finally merge this one.

@pcarruscag : let's finish up #653 as a next step and get that merged asap. I can help fix up any conflicts in the matrix classes.

TobiKattmann

A bit late to the party, I guess...
I thought i better submit that now instead of not submitting at all.
Most of it is quite pedantic.

TobiKattmann · 2019-03-21T14:26:52Z

Common/include/config_structure.hpp

@@ -144,6 +144,7 @@ class CConfig {
  Sens_Remove_Sharp,			/*!< \brief Flag for removing or not the sharp edges from the sensitivity computation. */
  Hold_GridFixed,	/*!< \brief Flag hold fixed some part of the mesh during the deformation. */
  Axisymmetric, /*!< \brief Flag for axisymmetric calculations */
+  TaylorGreen,


Description missing

TobiKattmann · 2019-03-21T14:27:37Z

Common/include/config_structure.hpp

@@ -5986,6 +5987,8 @@ class CConfig {
   */
  bool GetAxisymmetric(void);

+    bool GetTaylorGreen(void);


Description missing + indentation

TobiKattmann · 2019-03-21T14:31:22Z

Common/include/dual_grid_structure.hpp

  bool Domain,                        /*!< \brief Indicates if a point must be computed or belong to another boundary */
  Boundary,                           /*!< \brief To see if a point belong to the boundary (including MPI). */
  PhysicalBoundary,                   /*!< \brief To see if a point belong to the physical boundary (without includin MPI). */
-  SolidBoundary;                      /*!< \brief To see if a point belong to the physical boundary (without includin MPI). */
+  SolidBoundary,                      /*!< \brief To see if a point belong to the physical boundary (without includin MPI). */


Copied description.
including instead includin.
Difference between solid and physical not clear to me.

TobiKattmann · 2019-03-21T14:43:56Z

Common/include/geometry_structure.hpp

@@ -380,7 +421,86 @@ class CGeometry {
 	 * \brief Destructor of the class.
 	 */
 	virtual ~CGeometry(void);
+
+  /*!


2x brief, not sure if that has a meaning within doxygen?

TobiKattmann · 2019-03-21T14:49:07Z

Common/include/geometry_structure.hpp

@@ -715,6 +835,12 @@ class CGeometry {
 	 */
 	virtual void MatchInterface(CConfig *config);

+  /*!
+   * \brief A virtual member.
+   * \param[in] config - Definition of the particular problem.


Missing input val_periodic. Can be copied from the non-virtual MatchPeriodic from l. 2119

TobiKattmann · 2019-05-24T08:14:51Z

Common/include/geometry_structure.hpp

+   * \param[in] config - Definition of the particular problem.
+   * \param[in] val_periodic - Index of the first periodic face in a pair.
+   */
+  void MatchPeriodic(CConfig *config, unsigned short val_periodic);


Input parameters as a list

TobiKattmann · 2019-05-24T08:15:12Z

Common/include/linear_solvers_structure.hpp

@@ -191,10 +191,11 @@ class CSysSolve {
   * \param[in] tol - tolerance with which to solve the system
   * \param[in] m - maximum size of the search subspace
   * \param[in] monitoring - turn on priting residuals from solver to screen.
+   * \param[in] config - Definition of the particular problem.
   */
  unsigned long CG_LinSolver(const CSysVector & b, CSysVector & x, CMatrixVectorProduct & mat_vec,


Input parameters as a list

TobiKattmann · 2019-05-24T08:15:19Z

Common/include/linear_solvers_structure.hpp

@@ -206,10 +207,11 @@ class CSysSolve {
   * \param[in] m - maximum size of the search subspace
   * \param[in] residual
   * \param[in] monitoring - turn on priting residuals from solver to screen.
+   * \param[in] config - Definition of the particular problem.
   */
  unsigned long FGMRES_LinSolver(const CSysVector & b, CSysVector & x, CMatrixVectorProduct & mat_vec,


Input parameters as a list

TobiKattmann · 2019-05-24T08:15:24Z

Common/include/linear_solvers_structure.hpp

@@ -221,10 +223,11 @@ class CSysSolve {
   * \param[in] m - maximum size of the search subspace
   * \param[in] residual
   * \param[in] monitoring - turn on priting residuals from solver to screen.
+   * \param[in] config - Definition of the particular problem.
   */
  unsigned long BCGSTAB_LinSolver(const CSysVector & b, CSysVector & x, CMatrixVectorProduct & mat_vec,


Input parameters as a list

TobiKattmann · 2019-05-24T08:15:37Z

Common/src/config_structure.cpp

@@ -57,7 +57,7 @@ vector<double> GEMM_Profile_MaxTime;      /*!< \brief Maximum time spent for thi
 #include "../include/ad_structure.hpp"
 #include "../include/toolboxes/printing_toolbox.hpp"

-CConfig::CConfig(char case_filename[MAX_STRING_SIZE], unsigned short val_software, unsigned short val_iZone, unsigned short val_nZone, unsigned short val_nDim, unsigned short verb_level) {
+CConfig::CConfig(char case_filename[MAX_STRING_SIZE], unsigned short val_software, unsigned short val_iZone, unsigned short val_nZone, unsigned short val_nDim, bool verb_high) {


Input parameters as a list

economon added 30 commits June 12, 2018 11:58

Basic implementation of entirely new periodic BC for Euler. Needs to …

9d0c59a

…be generalized to all solvers and parallelization should be improved.

Updating with develop.

223a82e

Prototype routine for communicating gradients at periodic boundaries.

44d5233

Merged with develop and fixed conflicts.

fdcc27e

Periodic BCs now handle gradients, limiters, undivided laplacian, sen…

8f57b71

…sors, and eigenvalues. Both Euler and N-S operational. Fixed multiple bugs.

Merge branch 'develop' into feature_periodic

f306b5b

Added vorticity and q-criterion to output for testing periodic BC.

2a24b3f

Merge branch 'develop' into feature_periodic

5c99356

Added periodic BC routines to incompressible solver. Needs testing.

ef123aa

Some fixes for incompressible periodic BCs.

2b2a113

Merge branch 'develop' into feature_periodic

b2be497

First prototype periodic BC version for RANS.

6718b48

Merged develop and fixed conflicts.

02f59e1

Merge branch 'fix_xcode' into feature_periodic

97e5240

Moved over rotation matrix routines.

3673357

Merge branch 'develop' into feature_periodic

8e36413

Added geometry routines for P2P comms.

a501552

Added matrix routines for P2P comms.

dfd9655

Added solver routines for P2P comms.

0d832d6

Added comm level to replace verbosity.

b003c1e

Activated new mpi comms for matrix class only.

c648de4

Added comm level full as default.

daca8f5

Removed unused mpi comm routines.

721282d

Adjusted preprocessing for non-CFD modules.

4b6f3e5

Activated new P2P routines for geometry and solver classes.

b3995dc

Small change.

c521fa4

Comm level checks to save mpi overhead when requested.

2685cb5

Fixed problem with MPI_Waitall.

123a635

Update with develop and fixed conflicts.

700dd7c

Creating new MPI structures for periodicity.

abbefd2

Updating all regression test values.

2631bca

Removing unused function.

d0671aa

EduardoMolina reviewed Apr 22, 2019

View reviewed changes

SU2_CFD/src/driver_structure.cpp Outdated Show resolved Hide resolved

rsanfer approved these changes Apr 23, 2019

View reviewed changes

economon added 2 commits April 23, 2019 12:00

Merge branch 'develop' into feature_mpi_periodic

8d9ed38

Merge and fixed conditional to avoid setting FV periodic data structu…

1140145

…res with the DG solver.

economon merged commit 1571f0c into develop Apr 24, 2019

economon deleted the feature_mpi_periodic branch April 24, 2019 01:27

This was referenced Apr 30, 2019

Order of parallel communication #252

Closed

Current status of periodic BCs #416

Closed

Segfault when running problem with periodic boundary conditions #431

Closed

MPI Failure when Running with 16+ Cores #550

Closed

TobiKattmann reviewed May 24, 2019

View reviewed changes

This was referenced Jun 5, 2019

Remove Dead Code 2 #699

Merged

Remove Dead Code 3 #701

Merged

pcarruscag mentioned this pull request Jul 30, 2019

Small fix affecting MPI+MG+RANS #744

Merged

4 tasks

clarkpede mentioned this pull request Aug 21, 2019

Periodic boundary conditions and simple timestepping don't combine well #763

Closed

TobiKattmann mentioned this pull request Aug 26, 2019

Streamwise periodicity for incompressible flow #773

Merged

4 tasks

talbring added changelog:chore changelog:feature and removed changelog:chore labels Nov 7, 2019

talbring changed the title ~~MPI Point-to-Point Refactoring + New Periodic BC~~ MPI Point-to-Point Refactoring + New Periodic BC Implementation Nov 8, 2019

pr-triage bot added the PR: merged label Nov 8, 2019

pcarruscag mentioned this pull request Feb 29, 2020

SU2 v7 is slower on Ethernet cluster #894

Closed

cvencro mentioned this pull request Aug 16, 2020

Fixes for incompressible solver - rotating frame and convergence rate for unsteady problems #1067

Merged

5 tasks

TobiKattmann mentioned this pull request Sep 26, 2021

Add periodic boundary conditions to the heat-equation solver #1382

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPI Point-to-Point Refactoring + New Periodic BC Implementation #652

MPI Point-to-Point Refactoring + New Periodic BC Implementation #652

economon commented Feb 15, 2019 •

edited

Loading

economon commented Apr 19, 2019

EduardoMolina commented Apr 20, 2019

rsanfer left a comment

economon commented Apr 23, 2019

economon commented Apr 24, 2019

TobiKattmann left a comment

TobiKattmann Mar 21, 2019

TobiKattmann Mar 21, 2019

TobiKattmann Mar 21, 2019

TobiKattmann Mar 21, 2019

TobiKattmann Mar 21, 2019

TobiKattmann May 24, 2019

TobiKattmann May 24, 2019

TobiKattmann May 24, 2019

TobiKattmann May 24, 2019

TobiKattmann May 24, 2019

MPI Point-to-Point Refactoring + New Periodic BC Implementation #652

MPI Point-to-Point Refactoring + New Periodic BC Implementation #652

Conversation

economon commented Feb 15, 2019 • edited Loading

Proposed Changes

Related Work

PR Checklist

economon commented Apr 19, 2019

EduardoMolina commented Apr 20, 2019

rsanfer left a comment

Choose a reason for hiding this comment

economon commented Apr 23, 2019

economon commented Apr 24, 2019

TobiKattmann 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

economon commented Feb 15, 2019 •

edited

Loading