[WIP] fix multigrid agglomeration #2375

bigfooted · 2024-11-08T09:35:14Z

Proposed Changes

Implement multigrid agglomeration rules according to Nishikawa et al paper:

https://www.researchgate.net/publication/267557097_Development_and_Application_of_Parallel_Agglomerated_Multigrid_Method_for_Complex_Geometries

These rules were not consistently implemented. Issues:

Ridge -> ridge agglomeration was not possible.
Interior points were allowed to be agglomerated with boundary points.
The symmetry plane / euler wall agglomeration rules are too restrictive.

TO DO: properly agglomerate nodes that are on mpi interfaces (SEND_RECEIVE markers)

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 --warnlevel=3 when using meson).
My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
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.

bigfooted · 2024-11-08T09:37:02Z

Common/include/geometry/CMultiGridGeometry.hpp

@@ -45,7 +45,7 @@ class CMultiGridGeometry final : public CGeometry {
   * \param[in] config - Definition of the particular problem.
   * \return <code>TRUE</code> or <code>FALSE</code> depending if the control volume can be agglomerated.
   */
-  bool SetBoundAgglomeration(unsigned long CVPoint, short marker_seed, const CGeometry* fine_grid,
+  bool SetBoundAgglomeration(unsigned long CVPoint, vector<short> marker_seed, const CGeometry* fine_grid,


We need to pass information about all markers on the seed node (number of markers and index of marker)

Common/src/geometry/CMultiGridGeometry.cpp

+        //if ((config->GetMarker_All_KindBC(marker_seed[0]) == SYMMETRY_PLANE) ||
+        //    (config->GetMarker_All_KindBC(marker_seed[0]) == EULER_WALL)) {
+         // if (config->GetMarker_All_KindBC(copy_marker[0]) == SEND_RECEIVE) {
+         //   agglomerate_CV = false;
+         // }
+        //}


To fix the commented-out code error, remove the problematic commented-out code block entirely (lines 558–567). This will eliminate dead code and reduce confusion for future maintainers. Do not replace it with documentation, as the lines in question are pure C++ code, not explanatory comments. Only remove the commented-out code, leaving all surrounding code unchanged.

No new imports, method definitions, or dependencies are needed. The only required update is to the region in Common/src/geometry/CMultiGridGeometry.cpp where the commented-out code appears, specifically lines 558–567.

Common/src/geometry/CMultiGridGeometry.cpp

+          //if ((config->GetMarker_All_KindBC(copy_marker[0]) == EULER_WALL) ||
+          //    (config->GetMarker_All_KindBC(copy_marker[1]) == EULER_WALL)) {
+            agglomerate_seed = true;
+          //}


To fix this issue, all commented-out code (including the //} on line 137 and other fragments thereof) should either be fully restored if required, or removed entirely for clarity. In this specific case, the disabled logic concerns whether to allow agglomeration when either of two markers correspond to the EULER_WALL type. The best option is to remove the commented-out code (//if...., // ..., //}), since the current active logic always sets agglomerate_seed = true; for counter == 2, and thus the commented code is not contributing any functionality or documentation. This increases code readability and maintainability.

Edits should be limited to the lines 122–137 in Common/src/geometry/CMultiGridGeometry.cpp: all commented-out code lines within that block should be removed. Context lines above and below should be preserved, with no change to the current (active) logic or function.

No imports, methods, or definitions need be added.

Common/src/geometry/CMultiGridGeometry.cpp

+        //if (config->GetMarker_All_KindBC(copy_marker[0]) == SEND_RECEIVE) {
+        //  agglomerate_CV = true;
+        //}


To fix the problem, we should remove the commented-out code entirely from this region, while retaining any actual commentary provided in regular comments. In this case, lines 558 to 560 are commented-out C++ code (conditional check and assignment) and can be cleanly removed without affecting the functional comments or overall code logic, as the conditions appear to be covered elsewhere. Care should be taken not to remove any comments that describe logic, only remove actual deactivated code. The changes are to be made in file Common/src/geometry/CMultiGridGeometry.cpp, lines 558–560. No new imports or definitions are needed.

Common/src/geometry/CMultiGridGeometry.cpp

+      //      agglomerate_CV = true;
+      //    }
+      //  }


The best way to fix this problem is to remove the lines that contain commented-out code—specifically, the line // agglomerate_CV = true; at line 591 and related commented-out code in that block (lines 585–593). Retain any comments that provide meaningful documentation, but extraneous commented-out code should be deleted. Only edit the affected function within the provided range.

SU2_CFD/src/solvers/CSolverFactory.cpp

@@ -307,6 +307,7 @@
    case SUB_SOLVER_TYPE::TURB_SST:
      genericSolver = CreateTurbSolver(kindTurbModel, solver, geometry, config, iMGLevel, false);
      metaData.integrationType = INTEGRATION_TYPE::SINGLEGRID;
+      //metaData.integrationType = INTEGRATION_TYPE::MULTIGRID;


To fix the issue, remove the commented-out code line entirely. Since commented-out code such as //metaData.integrationType = INTEGRATION_TYPE::MULTIGRID; does not serve as documentation or provide necessary context and does not affect the current functionality, simply deleting the line is the best fix. This edit should be applied at line 313, leaving the remaining code untouched.

YairMO · 2024-11-16T05:57:23Z

Hi,

It would be great to make the MG work better. I would like to take this opportunity to improve my understanding of the MG inputs in the cfg file. Here is an example of MG input:

MGLEVEL=3
MGCYCLE= V_CYCLE
MG_PRE_SMOOTH= ( 5, 5, 5, 10 )
MG_POST_SMOOTH= ( 5, 5, 5, 5 )
MG_CORRECTION_SMOOTH= ( 25, 20, 15, 10 )

I was expecting to obtain the same report in the SU2 output file. However, there seems to be a gap between the input and the output reports. I attached the corresponding report output (of the above input):

Also, could you tell me how a turbulence model is handled within the MG? Is it projected or solved using the MG method?

Thank you

bigfooted · 2024-11-18T14:35:57Z

In SU2, we overwrite part of the config settings. The nr of pre-smoothing steps for the finest grid is always 1. The nr of post-smoothing steps for the coarsest and finest grid is always 0. I do not know why.
At the moment, we do not use multigrid for additional scalar equations like turbulence and species transport.

YairMO · 2024-11-18T19:07:56Z

Thank you.
I understand why pre- and post-smoothing of the coarsest and finest levels may be indistinguishable. Therefore, I understand why the post-smoothing of these levels is zero, but I expect instead to control the pre-smoothing relaxation of the finest level. Fixing the pre-smoothing of the finest level to 1 relaxation may be insufficient.

We wish to contribute our knowledge about MG for scalar transport equations; if an exciting group is on this topic, we will be happy to collaborate.

bigfooted · 2024-11-19T08:20:15Z

Nice to hear there is a larger interest in getting the multigrid method to a higher level. If you would like to enable multigrid for species transport, the first thing to do is change the integration type from SINGLEGRID to MULTIGRID for CreateSpeciesSolver in CSolverFactory.cpp. Then in CMultigridIntegration.cpp, there might be some solver specific things that need to be added.
Another thing that seems important for viscous applications is to agglomerate along implicit lines normal to viscous walls.

YairMO · 2024-11-19T08:31:59Z

Thank you for pointing this out. Does the current agglomeration (for the mean-flow equations) consider lines normal to the surface? I think that the Hiroaki paper (MG) considred implicit line?

bigfooted · 2024-11-19T18:00:41Z

Hi, No we currently do not do agglomeration along lines. I suspect this is one reason that we do not have good multigrid performance for a number of viscous testcases. @EvertBunschoten was looking into this as well, but he was not yet able to reproduce the good performance reported by the papers of Diskin, Nishikawa and others.

initial commit

23ac38b

bigfooted added PR: draft changelog:fix labels Nov 8, 2024

bigfooted commented Nov 8, 2024

View reviewed changes

github-advanced-security bot found potential problems Nov 8, 2024

View reviewed changes

bigfooted added 2 commits November 8, 2024 14:02

fix vector

a47c0f4

update agglomeration

f846cbb

github-advanced-security bot found potential problems Nov 15, 2024

View reviewed changes

Merge branch 'develop' into fix_multigrid_agglomeration

f2fb97e

bigfooted added 6 commits December 22, 2024 13:56

Merge branch 'develop' into fix_multigrid_agglomeration

70a0638

Merge branch 'develop' into fix_multigrid_agglomeration

711ba29

Merge branch 'develop' into fix_multigrid_agglomeration

1953dfc

Merge branch 'develop' into fix_multigrid_agglomeration

6d23aae

Merge branch 'develop' into fix_multigrid_agglomeration

ee86d12

Merge branch 'develop' into fix_multigrid_agglomeration

8e14cf5

@@ -119,22 +119,15 @@
                       agglomerate_seed = true;
                       /*--- Euler walls can be curved and agglomerating them leads to difficulties ---*/
-                      //if (config->GetMarker_All_KindBC(marker_seed[0]) == EULER_WALL)
-                      //  agglomerate_seed = false;
                     }
                     /*--- If there are two markers, we will agglomerate if any of the
                      markers is SEND_RECEIVE ---*/
                     if (counter == 2) {
-                      //agglomerate_seed = (config->GetMarker_All_KindBC(copy_marker[0]) == SEND_RECEIVE) ||
-                      //                   (config->GetMarker_All_KindBC(copy_marker[1]) == SEND_RECEIVE);
                       /* --- Euler walls can also not be agglomerated when the point has 2 markers ---*/
-                      //if ((config->GetMarker_All_KindBC(copy_marker[0]) == EULER_WALL) ||
-                      //    (config->GetMarker_All_KindBC(copy_marker[1]) == EULER_WALL)) {
                         agglomerate_seed = true;
-                      //}
                     }
                     /*--- If there are more than 2 markers, the agglomeration will be discarded ---*/

@@ -582,15 +582,7 @@
                     }
                     /*--- First we verify that the seed is a physical boundary ---*/
-                  //  if (config->GetMarker_All_KindBC(marker_seed[0]) != SEND_RECEIVE) {
-                  //    /*--- Then we check that one of the markers is equal to the seed marker, and the other is send/receive ---*/
-                  //    if (((copy_marker[0] == marker_seed[0]) && (config->GetMarker_All_KindBC(copy_marker[1]) == SEND_RECEIVE)) ||
-                  //        ((config->GetMarker_All_KindBC(copy_marker[0]) == SEND_RECEIVE) && (copy_marker[1] == marker_seed[0]))) {
                     agglomerate_CV = false;
-                  //      agglomerate_CV = true;
-                  //    }
-                  //  }
                   }
                 }
                 /*--- If the element belongs to the domain, it is never agglomerated. ---*/

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] fix multigrid agglomeration #2375

[WIP] fix multigrid agglomeration #2375

Uh oh!

bigfooted commented Nov 8, 2024

Uh oh!

bigfooted Nov 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Copilot Autofix

Check notice

Copilot Autofix

Check notice

Copilot Autofix

Check notice

Copilot Autofix

Check notice

Copilot Autofix

YairMO commented Nov 16, 2024

Uh oh!

bigfooted commented Nov 18, 2024

Uh oh!

YairMO commented Nov 18, 2024

Uh oh!

bigfooted commented Nov 19, 2024

Uh oh!

YairMO commented Nov 19, 2024

Uh oh!

bigfooted commented Nov 19, 2024

Uh oh!

Uh oh!

[WIP] fix multigrid agglomeration #2375

Are you sure you want to change the base?

[WIP] fix multigrid agglomeration #2375

Uh oh!

Conversation

bigfooted commented Nov 8, 2024

Proposed Changes

PR Checklist

Uh oh!

bigfooted Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Copilot Autofix

Check notice

Copilot Autofix

Check notice

Copilot Autofix

Check notice

Copilot Autofix

Check notice

Copilot Autofix

YairMO commented Nov 16, 2024

Uh oh!

bigfooted commented Nov 18, 2024

Uh oh!

YairMO commented Nov 18, 2024

Uh oh!

bigfooted commented Nov 19, 2024

Uh oh!

YairMO commented Nov 19, 2024

Uh oh!

bigfooted commented Nov 19, 2024

Uh oh!

Uh oh!