Fix multigrid agglomeration #2712

bigfooted · 2026-01-18T14:55:07Z

Proposed Changes

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

Implement multigrid agglomeration according to Nishikawa and Diskin.

euler walls are working correctly.
mpi still needs to be checked.
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.

Common/src/geometry/CMultiGridGeometry.cpp

SU2_CFD/src/integration/CMultiGridIntegration.cpp

bigfooted · 2026-01-19T09:44:44Z

TestCases/euler/channel/inv_channel_RK.cfg

+MG_PRE_SMOOTH= ( 4, 4, 4, 4 )
+MG_POST_SMOOTH= ( 4, 4, 4, 4 )
+MG_CORRECTION_SMOOTH= ( 4, 4, 4, 4 )
+MG_DAMP_RESTRICTION= 0.5
+MG_DAMP_PROLONGATION= 0.5


When MG is finalized, these will be initial values and will be adaptively changed.

Common/src/geometry/CMultiGridGeometry.cpp

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

…ix_MG_part_1

Common/src/geometry/CMultiGridGeometry.cpp

+          if (nDim == 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 = false;
+          }


In general, a futile conditional with an empty then-branch and no else-branch should be removed or rewritten so that the condition is either used meaningfully or not evaluated at all. Any explanatory comments should be kept outside of a non-functional if to avoid confusing tools and readers.

For this case in Common/src/geometry/CMultiGridGeometry.cpp, the if (nDim == 2) on line 178 has its logic entirely commented out. The behavior for nDim == 2 is therefore already "do nothing", and the condition serves only as a disabled skeleton. To fix it without changing existing functionality, we should delete the empty if statement (lines 178–181) and retain the commented-out description (or a short note) as plain comments directly in the counter == 2 block. The rest of the logic—especially the if (nDim == 3) agglomerate_seed = (copy_marker[0] == copy_marker[1]); and the curvature-based checks—must remain unchanged.

No new methods, imports, or definitions are needed. The edit is a local cleanup of the empty if block within the shown region.

Common/src/geometry/CMultiGridGeometry.cpp

+        // /*--- If seed has 2 markers and one is SEND_RECEIVE, do not agglomerate with physical boundary ---*/
+        // if ((marker_seed.size() == 2) && (config->GetMarker_All_KindBC(marker_seed[0]) == SEND_RECEIVE)) {
+        //   if (copy_marker[0] == marker_seed[1]) {
+        //     agglomerate_CV = false;
+        //   }
+        // }
+        // if ((marker_seed.size() == 2) && (config->GetMarker_All_KindBC(marker_seed[1]) == SEND_RECEIVE)) {
+        //   if (copy_marker[0] == marker_seed[0]) {
+        //     agglomerate_CV = false;
+        //   }
+        // }


To fix the problem, we should remove the commented-out code block and, if necessary, replace it with a brief natural-language comment that does not look like code. This avoids confusion, keeps the file clean, and aligns with the recommendation to remove or reinstate commented-out code.

Concretely, in Common/src/geometry/CMultiGridGeometry.cpp, within the if (counter == 1) branch around line 840, delete the commented-out if statements and their bodies (lines 846–856) while leaving the surrounding logic intact. Since this code was already disabled with //, removing it does not alter existing functionality and requires no additional methods, imports, or definitions. If you want to preserve the intent, you can leave or slightly rephrase the existing explanatory comments that are not code-like; in the snippet given, the descriptive comments at 838–843 and 858–860 are already adequate, so we can simply drop the commented-out logic.

SU2_CFD/src/integration/CMultiGridIntegration.cpp

Common/src/geometry/CMultiGridGeometry.cpp

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

…ix_MG_part_1

jblueh · 2026-02-02T20:08:51Z

SU2_CFD/src/integration/CMultiGridIntegration.cpp

+      /*--- Force sequential execution of recursive calls for determinism ---*/
+      SU2_OMP_BARRIER
+      SU2_OMP_MASTER
+      {
+        // Empty - just ensures only one thread enters the recursion at a time
+      }
+      END_SU2_OMP_MASTER
+      SU2_OMP_BARRIER
+


What's the intent here? Right now, it seems equivalent to a plain SU2_OMP_BARRIER.

Thanks Johannes, for looking into this. I was testing some OMP stuff because it was giving me non-deterministic results (different convergence nr of iterations per run). I will clean this up as well.

jblueh · 2026-02-02T22:16:09Z

SU2_CFD/src/integration/CMultiGridIntegration.cpp

+      /*--- Force sequential execution of recursive calls for determinism ---*/
+      SU2_OMP_BARRIER


Is the leading comment still up to date then? Maybe something like

Suggested change

/*--- Force sequential execution of recursive calls for determinism ---*/

SU2_OMP_BARRIER

/*--- Synchronize prior to recursive calls for determinism ---*/

SU2_OMP_BARRIER

pcarruscag · 2026-02-03T10:02:37Z

Common/include/geometry/CMultiGridGeometry.hpp

 /*!
 * \class CMultiGridGeometry
- * \brief Class for defining the multigrid geometry, the main delicated part is the
+ * \brief Class for defining the multigrid geometry, the main delegated part is the


pcarruscag · 2026-02-03T10:03:36Z

Common/src/geometry/dual_grid/CPoint.cpp

  /*--- Multigrid structures. ---*/
  if (config->GetnMGLevels() > 0) {
-    Parent_CV.resize(npoint) = 0;
+    Parent_CV.resize(npoint) = ULONG_MAX;  // Initialize to ULONG_MAX to indicate unagglomerated


std::numeric_limits::max()

pcarruscag · 2026-02-03T10:04:08Z

SU2_CFD/include/integration/CMultiGridIntegration.hpp

   */
  void SetRestricted_Solution(unsigned short RunTime_EqSystem, CSolver *sol_fine, CSolver *sol_coarse,
-                              CGeometry *geo_fine, CGeometry *geo_coarse, CConfig *config);
+                              CGeometry *geo_fine, CGeometry *geo_coarse, CConfig *config, unsigned short iMesh = 999);


why the magic 999?

pcarruscag · 2026-02-03T10:05:01Z

Common/src/geometry/CMultiGridGeometry.cpp

   3rd) One marker ---> Surface (always agglomerate)
   4th) No marker ---> Internal Volume (always agglomerate) ---*/

+  // note that for MPI, we introduce interfaces and we can choose to have agglomeration over


Suggested change

// note that for MPI, we introduce interfaces and we can choose to have agglomeration over

// Note that for MPI, we introduce interfaces and we can choose to have agglomeration over

pcarruscag · 2026-02-03T10:07:44Z

Common/src/geometry/CMultiGridGeometry.cpp

+            // agglomerate_seed = ((config->GetMarker_All_KindBC(copy_marker[0]) == SEND_RECEIVE) ||
+            //                     (config->GetMarker_All_KindBC(copy_marker[1]) == SEND_RECEIVE));


wip? or the comment above if counter == 2 needs to be updated?

pcarruscag · 2026-02-03T10:51:56Z

SU2_CFD/src/integration/CMultiGridIntegration.cpp

+
+      /*--- Reset state at the beginning of a new solve (iter 0 or 1) ---*/
+      /*--- This ensures deterministic behavior across multiple runs ---*/
+      static unsigned long last_reset_iter = ULONG_MAX;


yeah this gets a bit out of hand xD

pcarruscag · 2026-02-03T10:52:57Z

SU2_CFD/src/integration/CMultiGridIntegration.cpp

+    /*--- Update LocalCFL at each coarse grid point ---*/
+    SU2_OMP_FOR_STAT(roundUpDiv(geometry_coarse->GetnPoint(), omp_get_num_threads()))
+    for (auto iPoint = 0ul; iPoint < geometry_coarse->GetnPoint(); iPoint++) {
+      solver_coarse->GetNodes()->SetLocalCFL(iPoint, CFL_coarse_new);


Make a function for the adaptation, please.

pcarruscag · 2026-02-03T10:55:09Z

SU2_CFD/src/integration/CMultiGridIntegration.cpp


      }
+
+      /*--- MPI sync after RK stage to ensure halos have updated solution for next smoothing iteration ---*/


I don't see why this would be necessary, in the whole code we always communicate everything after it is computed, so this is either missing somewhere else, or masking another issue.

pcarruscag · 2026-02-03T10:56:05Z

SU2_CFD/src/integration/CMultiGridIntegration.cpp


  delete [] Solution;

+  /*--- Ensure all threads complete before MPI communication ---*/


Not needed, for loops have an implied barrier at the end.

pcarruscag · 2026-02-03T10:56:56Z

SU2_CFD/src/integration/CMultiGridIntegration.cpp

+  /*--- Ensure MPI completion visible to all threads ---*/
+  SU2_OMP_BARRIER


Same comment about CompleteComms.
We use funneled communications, so I'm pretty sure we have a barrier somewhere in the comms.

bigfooted added 5 commits January 17, 2026 11:03

agglomeration only

8ef8e5c

agglomeration on mpi

ef5b3df

fix euler walls

7685588

remove debugging stuff

3d5cb12

helper function

e18050b

bigfooted added PR: draft changelog:fix labels Jan 18, 2026

bigfooted added 4 commits January 18, 2026 16:40

before helper function

2a70b87

remove debug info

69e5d5b

precommit

40b8638

remove implicit lines

bb3a25a

github-advanced-security bot found potential problems Jan 18, 2026

View reviewed changes

bigfooted added 6 commits January 18, 2026 18:48

cleanup commented stuff

c58caf9

precommit

ec0bf2e

cleanup

7ab2ea9

cleanup

6049a59

cleanup

b60fe4a

precommit

4bcc049

github-advanced-security bot found potential problems Jan 18, 2026

View reviewed changes

Common/src/geometry/CMultiGridGeometry.cpp Fixed Show fixed Hide fixed

bigfooted added 2 commits January 19, 2026 08:27

cleanup

2424d6e

add damping based on mesh level

626a242

github-advanced-security bot found potential problems Jan 19, 2026

View reviewed changes

SU2_CFD/src/integration/CMultiGridIntegration.cpp Fixed Show fixed Hide fixed

bigfooted added 2 commits January 19, 2026 10:39

small cleanup

42fa32d

Merge branch 'develop' into fix_MG_part_1

64cd69b

bigfooted commented Jan 19, 2026

View reviewed changes

bigfooted added 3 commits January 19, 2026 13:42

change CFL adjustment to safe value.

9845ff2

change CFL adjustment to safe value.

fc1de16

auto CFL adjustment for coarse meshes

457f160

github-advanced-security bot found potential problems Jan 19, 2026

View reviewed changes

Common/src/geometry/CMultiGridGeometry.cpp Fixed Show fixed Hide fixed

precommit

53a6c28

remove unused function

64a676b

github-advanced-security bot found potential problems Jan 26, 2026

View reviewed changes

Common/src/geometry/CMultiGridGeometry.cpp Fixed Show fixed Hide fixed

bigfooted and others added 5 commits January 26, 2026 14:02

Potential fix for code scanning alert no. 6152: Commented-out code

96caa0e

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Potential fix for code scanning alert no. 6153: Commented-out code

336aeae

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

fix codi constexpr is not a literal

4ecbde5

Merge branch 'fix_MG_part_1' of https://github.com/su2code/su2 into f…

d57ab24

…ix_MG_part_1

fix codi different type error

9bfcd71

bigfooted commented Jan 26, 2026

View reviewed changes

Common/src/geometry/CMultiGridGeometry.cpp Outdated Show resolved Hide resolved

bigfooted and others added 7 commits January 27, 2026 09:26

precommit

7e0ccf1

fix symmetry shared node

b63a428

fix constexpr

e86b74f

cleanup

f30060b

merge

4cdb106

Merge branch 'fix_symmetry' into fix_MG_part_1

135cdd5

fix adjoint local cfl

e1523ef

github-advanced-security bot found potential problems Jan 28, 2026

View reviewed changes

precommit

3269c08

github-advanced-security bot found potential problems Jan 28, 2026

View reviewed changes

Common/src/geometry/CMultiGridGeometry.cpp Fixed Show fixed Hide fixed

bigfooted and others added 4 commits January 28, 2026 17:06

Potential fix for code scanning alert no. 6158: Commented-out code

4be3e7c

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Potential fix for code scanning alert no. 6157: Commented-out code

b3c88fe

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

fix easy regression tests

cf8f7d1

Merge branch 'fix_MG_part_1' of https://github.com/su2code/su2 into f…

60d3441

…ix_MG_part_1

bigfooted removed the PR: draft label Jan 28, 2026

bigfooted changed the title ~~[WIP] Fix multigrid agglomeration~~ Fix multigrid agglomeration Jan 28, 2026

bigfooted added 2 commits January 29, 2026 14:28

merge, fix data race

208bf95

modify some OMP stuff

2c65015

jblueh reviewed Feb 2, 2026

View reviewed changes

cleanup some omp

0082b3e

jblueh reviewed Feb 2, 2026

View reviewed changes

pcarruscag reviewed Feb 3, 2026

View reviewed changes

@@ -174,11 +174,10 @@
                     /*--- Note that in 2D, this is a corner and we do not agglomerate unless they both are SEND_RECEIVE. ---*/
                     /*--- In 3D, we agglomerate if the 2 markers are the same. ---*/
                     if (counter == 2) {
-                      /*--- agglomerate if one of the 2 markers are MPI markers. ---*/
-                      if (nDim == 2) {
-                        // agglomerate_seed = ((config->GetMarker_All_KindBC(copy_marker[0]) == SEND_RECEIVE) ||
-                        //                     (config->GetMarker_All_KindBC(copy_marker[1]) == SEND_RECEIVE));
-                      }
+                      /*--- agglomerate if one of the 2 markers are MPI markers (2D case, currently disabled). ---*/
+                      // agglomerate_seed = ((nDim == 2) &&
+                      //                     ((config->GetMarker_All_KindBC(copy_marker[0]) == SEND_RECEIVE) ||
+                      //                      (config->GetMarker_All_KindBC(copy_marker[1]) == SEND_RECEIVE)));
                       /*--- agglomerate if both markers are the same. ---*/
                       if (nDim == 3) agglomerate_seed = (copy_marker[0] == copy_marker[1]);

		/--- Force sequential execution of recursive calls for determinism ---/
		SU2_OMP_BARRIER

	// note that for MPI, we introduce interfaces and we can choose to have agglomeration over
	// Note that for MPI, we introduce interfaces and we can choose to have agglomeration over

		// agglomerate_seed = ((config->GetMarker_All_KindBC(copy_marker[0]) == SEND_RECEIVE) \|\|
		// (config->GetMarker_All_KindBC(copy_marker[1]) == SEND_RECEIVE));


		}

		/--- MPI sync after RK stage to ensure halos have updated solution for next smoothing iteration ---/


		delete [] Solution;

		/--- Ensure all threads complete before MPI communication ---/

		/--- Ensure MPI completion visible to all threads ---/
		SU2_OMP_BARRIER

Fix multigrid agglomeration #2712

Are you sure you want to change the base?

Fix multigrid agglomeration #2712

Conversation

bigfooted commented Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Copilot Autofix

Check notice

Copilot Autofix

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigfooted Feb 2, 2026 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bigfooted commented Jan 18, 2026 •

edited

Loading

bigfooted Feb 2, 2026 •

edited

Loading