Linear solver changes to support hybrid parallel AD by pcarruscag · Pull Request #1228 · su2code/SU2

pcarruscag · 2021-03-11T20:04:55Z

Proposed Changes

Instead of computing transposed matrix vector products, which are harder to parallelize with OpenMP, and having special handling for transposed preconditioners, the matrix is transposed in place, then any preconditioner can be applied.
Some tweaks to support better Krylov discrete adjoint.

Related Work

#1214

PR Checklist

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, or simply --warnlevel=2 when using meson).
My contribution is commented and consistent with SU2 style.
I have added a test case that demonstrates my contribution, if necessary.
I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

pcarruscag · 2021-03-14T23:54:10Z

    /*--- Build preconditioner for the transposed Jacobian ---*/

+    if (RequiresTranspose) Jacobian.TransposeInPlace();
+


The transposition happens here.
Things that could go wrong with this approach:

Solving the same system multiple times, without clearing the matrix or reverting the transposition in between. It does not happen atm.

I think it is worth the risk, since all preconditioners can be used now, and re-transposing the matrix if necessary is not very expensive (probably the same as applying the ILU once).

Not that I used a preconditioner besides ILU for DA cases yet, but that is a nice thing 👍

TobiKattmann

Well, the code looks nice as always 💐 but I cannot really say much about the changes in general because I am not exactly familiar with that code :(

TobiKattmann · 2021-03-17T08:40:44Z

-  LinearToleranceType tol_type = LinearToleranceType::RELATIVE; /*!< \brief How the linear solvers interpret the tolerance. */
-  bool xIsZero = false;           /*!< \brief If true assume the initial solution is always 0. */
+  mutable LinearToleranceType tol_type = LinearToleranceType::ABSOLUTE; /*!< \brief How the linear solvers interpret the tolerance. */
+  mutable bool xIsZero = false;   /*!< \brief If true assume the initial solution is always 0. */


Where there is const, there must be mutable somewhere

Restarted FGMRES (which I put in a function) needs to force those values. But I'll do it some other way, mutable in this case is misleading as relevant state can be different after the solver is called.
I try to reserve mutable for variables that do not change the behavior of the object.

TobiKattmann · 2021-03-17T08:59:09Z

+    /*--- To keep the behavior of SU2_DOT, but not strictly required since jacobian is symmetric(?). ---*/
+    const bool RequiresTranspose = !mesh_deform || (config->GetKind_SU2() == SU2_DOT);


I guess SU2_DOT does a mesh deformation using the linear solvers so that is what the question mark refers to in the comment? Or am I missing sth here

Well the Jacobian is not symmetric because of how we do the boundary conditions but conjugate gradient still works so... 🤷

TobiKattmann · 2021-03-17T09:03:34Z

    /*--- Build preconditioner for the transposed Jacobian ---*/

+    if (RequiresTranspose) Jacobian.TransposeInPlace();
+


Not that I used a preconditioner besides ILU for DA cases yet, but that is a nice thing 👍

TobiKattmann · 2021-03-17T09:16:41Z

    da_sp_pinArray_cht_2d_dp_hf.cfg_file  = "DA_configMaster.cfg"
    da_sp_pinArray_cht_2d_dp_hf.test_iter = 100
-    da_sp_pinArray_cht_2d_dp_hf.test_vals = [-4.793283, -4.065832, -4.137121] #last 4 lines
+    da_sp_pinArray_cht_2d_dp_hf.test_vals = [-4.800583, -4.065533, -4.137185] #last 4 lines


Well the deviations are really tiny but both inc cases have INC_ENERGY_EQUATION= YES but discadj_cht is not here as the third case that has that... Not that I am worried but if you have an explanation why exactly those cases differ, I'd be interested?

The common denominator seemed to be cases that had a significant number of linear iterations.
I ran them to convergence and there is 0 difference in final results.
I will still test these changes in my optimization cases before merging.

TobiKattmann · 2021-03-19T13:59:37Z

 enum ENUM_LINEAR_SOLVER {
-  STEEPEST_DESCENT = 1,     /*!< \brief Steepest descent method for point inversion algoritm (Free-Form). */
-  NEWTON = 2,               /*!< \brief Newton method for point inversion algorithm (Free-Form). */
-  QUASI_NEWTON = 3,         /*!< \brief Quasi Newton method for point inversion algorithm (Free-Form). */
-  CONJUGATE_GRADIENT = 4,   /*!< \brief Preconditionated conjugate gradient method for grid deformation. */
-  FGMRES = 5,               /*!< \brief Flexible Generalized Minimal Residual method. */
-  BCGSTAB = 6,              /*!< \brief BCGSTAB - Biconjugate Gradient Stabilized Method (main solver). */
-  RESTARTED_FGMRES = 7,     /*!< \brief Flexible Generalized Minimal Residual method with restart. */
-  SMOOTHER = 8,             /*!< \brief Iterative smoother. */
-  PASTIX_LDLT = 9,          /*!< \brief PaStiX LDLT (complete) factorization. */
-  PASTIX_LU = 10,           /*!< \brief PaStiX LU (complete) factorization. */
+  CONJUGATE_GRADIENT,   /*!< \brief Preconditionated conjugate gradient method for grid deformation. */
+  FGMRES,               /*!< \brief Flexible Generalized Minimal Residual method. */
+  BCGSTAB,              /*!< \brief BCGSTAB - Biconjugate Gradient Stabilized Method (main solver). */
+  RESTARTED_FGMRES,     /*!< \brief Flexible Generalized Minimal Residual method with restart. */
+  SMOOTHER,             /*!< \brief Iterative smoother. */
+  PASTIX_LDLT,          /*!< \brief PaStiX LDLT (complete) factorization. */
+  PASTIX_LU,            /*!< \brief PaStiX LU (complete) factorization. */


@bigfooted likes that :D

…nsol_fixes

TobiKattmann · 2021-03-23T09:26:48Z

+    /*--- Reduce across all mpi ranks, only master thread communicates. ---*/
+    SU2_OMP_BARRIER
+    SU2_OMP_MASTER {
+      sum = dotRes;
+      const auto mpi_type = (sizeof(ScalarType) < sizeof(double)) ? MPI_FLOAT : MPI_DOUBLE;
+      SelectMPIWrapper<ScalarType>::W::Allreduce(&sum, &dotRes, 1, mpi_type, MPI_SUM, SU2_MPI::GetComm());


Resolves #1241 👍

TobiKattmann · 2021-03-24T10:28:21Z

    sp_pinArray_cht_2d_dp_hf.test_iter = 100
-    sp_pinArray_cht_2d_dp_hf.test_vals = [0.247022, -0.812199, -0.974877, -0.753315, 208.023676, 349.950000] #last 7 lines
+    sp_pinArray_cht_2d_dp_hf.test_vals = [0.247026, -0.811632, -0.982066, -0.753312, 208.023676, 350.180000] #last 7 lines
    sp_pinArray_cht_2d_dp_hf.su2_exec  = "mpirun -n 2 SU2_CFD"


I will redo the gradient validation on my side... and I like that commit message 👍

fixing the behavior of relative tolerance mode

6789ed6

pcarruscag added the changelog:feature label Mar 11, 2021

pr-triage Bot added the PR: draft label Mar 11, 2021

pcarruscag added 5 commits March 12, 2021 16:13

transpose the matrix instead of the product

b0f68f8

fix segfaults

579ee2e

factory method for preconditioners

4c2328a

remove transpose methods

e9180e9

more cleanup, update regressions

0b4e35e

pcarruscag changed the title ~~[WIP] Linear solver changes to support hybrid parallel AD~~ Linear solver changes to support hybrid parallel AD Mar 14, 2021

pcarruscag marked this pull request as ready for review March 14, 2021 23:40

pr-triage Bot added PR: unreviewed and removed PR: draft labels Mar 14, 2021

pcarruscag commented Mar 14, 2021

View reviewed changes

pcarruscag added 2 commits March 15, 2021 10:43

dead code

3a95af1

use default parameter for Comm Mode

0414355

TobiKattmann approved these changes Mar 17, 2021

View reviewed changes

pr-triage Bot added PR: reviewed-approved and removed PR: unreviewed labels Mar 17, 2021

no mutable

737170e

pr-triage Bot added PR: unreviewed and removed PR: reviewed-approved labels Mar 17, 2021

pcarruscag and others added 3 commits March 17, 2021 16:30

Merge remote-tracking branch 'upstream/develop' into linsol_fixes

14567f5

enum cleanup

62676bb

Merge branch 'develop' into linsol_fixes

0b29ab5

TobiKattmann reviewed Mar 19, 2021

View reviewed changes

pcarruscag and others added 5 commits March 22, 2021 17:21

efficiency and documentation tweaks

f9b56aa

Merge branch 'linsol_fixes' of https://github.com/su2code/SU2 into li…

79b7992

…nsol_fixes

fix #1241

b4f2772

happy updates

7c25134

Merge branch 'develop' into linsol_fixes

6f2230d

pcarruscag merged commit 949f4e5 into develop Mar 22, 2021

pcarruscag deleted the linsol_fixes branch March 22, 2021 23:01

pr-triage Bot added PR: merged and removed PR: unreviewed labels Mar 22, 2021

TobiKattmann reviewed Mar 23, 2021

View reviewed changes

pr-triage Bot added PR: unreviewed and removed PR: merged labels Mar 23, 2021

TobiKattmann mentioned this pull request Mar 23, 2021

SU2 freezes for a case with exactly 4 cores #1241

Closed

TobiKattmann reviewed Mar 24, 2021

View reviewed changes

Navigation Menu

Uh oh!

Linear solver changes to support hybrid parallel AD - #1228

Linear solver changes to support hybrid parallel AD#1228
pcarruscag merged 17 commits into
developfrom
linsol_fixes

pcarruscag commented Mar 11, 2021 •

edited

Loading

Uh oh!

pcarruscag Mar 14, 2021

Uh oh!

TobiKattmann Mar 17, 2021

Uh oh!

TobiKattmann left a comment

Uh oh!

TobiKattmann Mar 17, 2021

Uh oh!

pcarruscag Mar 17, 2021

Uh oh!

TobiKattmann Mar 17, 2021

Uh oh!

pcarruscag Mar 17, 2021

Uh oh!

TobiKattmann Mar 17, 2021

Uh oh!

TobiKattmann Mar 17, 2021

Uh oh!

pcarruscag Mar 17, 2021

Uh oh!

TobiKattmann Mar 19, 2021

Uh oh!

TobiKattmann Mar 23, 2021 •

edited

Loading

Uh oh!

TobiKattmann Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		/--- Build preconditioner for the transposed Jacobian ---/

		if (RequiresTranspose) Jacobian.TransposeInPlace();

		/--- To keep the behavior of SU2_DOT, but not strictly required since jacobian is symmetric(?). ---/
		const bool RequiresTranspose = !mesh_deform \|\| (config->GetKind_SU2() == SU2_DOT);

Uh oh!

Conversation

pcarruscag commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Related Work

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiKattmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pcarruscag commented Mar 11, 2021 •

edited

Loading

TobiKattmann Mar 23, 2021 •

edited

Loading