Skip to content

Fix multigrid agglomeration#2712

Open
bigfooted wants to merge 81 commits intodevelopfrom
fix_MG_part_1
Open

Fix multigrid agglomeration#2712
bigfooted wants to merge 81 commits intodevelopfrom
fix_MG_part_1

Conversation

@bigfooted
Copy link
Contributor

@bigfooted bigfooted commented Jan 18, 2026

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 seems to be working (can be improved)

  • OMP seems to be working (can be improved)

  • multigrid does not crash anymore

  • 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.

Comment on lines 491 to 503
/*--- Get current CFL values - only master thread reads to avoid concurrent access races.
All other threads wait at barrier to get the same cached values. ---*/
su2double CFL_fine = 0.0;
su2double CFL_coarse_current = 0.0;
su2double CFL_coarse_new = 0.0;

/*--- Compute adaptive CFL for coarse grid ---*/
su2double CFL_coarse_new = computeMultigridCFL(config, solver_coarse, geometry_coarse, iMesh, CFL_fine, CFL_coarse_current);
SU2_OMP_MASTER
{
CFL_fine = config->GetCFL(iMesh);
CFL_coarse_current = config->GetCFL(iMesh+1);

/*--- Compute adaptive CFL for coarse grid (master thread only) ---*/
CFL_coarse_new = computeMultigridCFL(config, solver_coarse, geometry_coarse, iMesh, CFL_fine, CFL_coarse_current);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can help you with the OMP stuff, just point me to a simple case with issues.
What you're doing here doesn't work because CFL_fine is a thread-local variable, so only the master thread will have a value here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank, pedro, I am still (a bit) lost here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I reverted this again, but this specific fix seems to solve the 'regular' thread sanitizer tests'. The AD thread sanitizer tests are still failing though.

Comment on lines +497 to +498
// passivedouble CFL_fine_passive = 0.0;
// passivedouble CFL_coarse_current_passive = 0.0;

Check notice

Code scanning / CodeQL

Commented-out code

This comment appears to contain commented-out code.

Copilot Autofix

AI 3 days ago

In general, to fix commented‑out code, either fully reinstate it as active code (if it represents the intended behavior) or remove it altogether, possibly replacing it with a concise explanatory comment if some of its intent is important for understanding.

Here, the multigrid CFL is currently set to a fixed value 1.0, and the previously more complex logic to read CFL values in a SU2_OMP_MASTER block is commented out. Since the active code clearly uses the fixed value and there is no guarantee that the old code is still correct, the lowest‑risk fix is to remove the commented‑out code completely, while retaining the surrounding synchronization comments which explain the barriers. Concretely, in SU2_CFD/src/integration/CMultiGridIntegration.cpp, around lines 495–505, delete the lines starting with // passivedouble CFL_fine_passive = 0.0; through // END_SU2_OMP_MASTER. No new methods, imports, or definitions are needed; we are only cleaning up dead commented‑out code.

Suggested changeset 1
SU2_CFD/src/integration/CMultiGridIntegration.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/SU2_CFD/src/integration/CMultiGridIntegration.cpp b/SU2_CFD/src/integration/CMultiGridIntegration.cpp
--- a/SU2_CFD/src/integration/CMultiGridIntegration.cpp
+++ b/SU2_CFD/src/integration/CMultiGridIntegration.cpp
@@ -494,15 +494,15 @@
 
     /*--- Read current CFL values in master thread to ensure thread-safe access.
           Extract passive values to avoid AD tape recording in parallel region. ---*/
-    // passivedouble CFL_fine_passive = 0.0;
-    // passivedouble CFL_coarse_current_passive = 0.0;
 
-    // SU2_OMP_MASTER
-    // {
-    //   CFL_fine_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh));
-    //   CFL_coarse_current_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh+1));
-    // }
-    // END_SU2_OMP_MASTER
+
+
+
+
+
+
+
+
     /*--- Implicit barrier here ensures all threads see the CFL values before proceeding ---*/
 
     /*--- Compute adaptive CFL for coarse grid using passive values (no tape recording) ---*/
EOF
@@ -494,15 +494,15 @@

/*--- Read current CFL values in master thread to ensure thread-safe access.
Extract passive values to avoid AD tape recording in parallel region. ---*/
// passivedouble CFL_fine_passive = 0.0;
// passivedouble CFL_coarse_current_passive = 0.0;

// SU2_OMP_MASTER
// {
// CFL_fine_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh));
// CFL_coarse_current_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh+1));
// }
// END_SU2_OMP_MASTER








/*--- Implicit barrier here ensures all threads see the CFL values before proceeding ---*/

/*--- Compute adaptive CFL for coarse grid using passive values (no tape recording) ---*/
Copilot is powered by AI and may make mistakes. Always verify output.
Comment on lines +500 to +505
// SU2_OMP_MASTER
// {
// CFL_fine_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh));
// CFL_coarse_current_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh+1));
// }
// END_SU2_OMP_MASTER

Check notice

Code scanning / CodeQL

Commented-out code

This comment appears to contain commented-out code.

Copilot Autofix

AI 3 days ago

In general, to fix commented-out code issues you either (a) remove the dead code permanently if it is no longer needed, or (b) reinstate it as active code (possibly refactored into a function), or (c) convert it into clearly non-executable documentation (e.g., in a Doxygen block or as pseudo-code with markers). Here, the simplest way to fix the issue without changing existing functionality is to remove the block of commented-out code that was implementing a more complex CFL computation, because the current implementation intentionally uses a fixed CFL_coarse_new = 1.0.

Concretely, in SU2_CFD/src/integration/CMultiGridIntegration.cpp, in the multigrid loop around lines 495–507, delete the commented-out declarations of CFL_fine_passive and CFL_coarse_current_passive, the commented SU2_OMP_MASTER block, and the associated explanatory comment about the implicit barrier that specifically references these commented variables. Keep the surrounding comments about synchronization that still apply to the remaining code. No new methods, imports, or definitions are required; we are just removing unused, commented-out code.

Suggested changeset 1
SU2_CFD/src/integration/CMultiGridIntegration.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/SU2_CFD/src/integration/CMultiGridIntegration.cpp b/SU2_CFD/src/integration/CMultiGridIntegration.cpp
--- a/SU2_CFD/src/integration/CMultiGridIntegration.cpp
+++ b/SU2_CFD/src/integration/CMultiGridIntegration.cpp
@@ -492,21 +492,8 @@
     /*--- Synchronize before reading CFL to avoid race with concurrent writes from computeMultigridCFL ---*/
     SU2_OMP_BARRIER
 
-    /*--- Read current CFL values in master thread to ensure thread-safe access.
-          Extract passive values to avoid AD tape recording in parallel region. ---*/
-    // passivedouble CFL_fine_passive = 0.0;
-    // passivedouble CFL_coarse_current_passive = 0.0;
+    /*--- Compute adaptive CFL for coarse grid (currently using a fixed value for debugging) ---*/
 
-    // SU2_OMP_MASTER
-    // {
-    //   CFL_fine_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh));
-    //   CFL_coarse_current_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh+1));
-    // }
-    // END_SU2_OMP_MASTER
-    /*--- Implicit barrier here ensures all threads see the CFL values before proceeding ---*/
-
-    /*--- Compute adaptive CFL for coarse grid using passive values (no tape recording) ---*/
-
     //nijso: just put a fixed value to see if the omp problem is really here...
     passivedouble CFL_coarse_new = 1.0; //computeMultigridCFL(config, solver_coarse, geometry_coarse, iMesh, CFL_fine_passive, CFL_coarse_current_passive);
 
EOF
@@ -492,21 +492,8 @@
/*--- Synchronize before reading CFL to avoid race with concurrent writes from computeMultigridCFL ---*/
SU2_OMP_BARRIER

/*--- Read current CFL values in master thread to ensure thread-safe access.
Extract passive values to avoid AD tape recording in parallel region. ---*/
// passivedouble CFL_fine_passive = 0.0;
// passivedouble CFL_coarse_current_passive = 0.0;
/*--- Compute adaptive CFL for coarse grid (currently using a fixed value for debugging) ---*/

// SU2_OMP_MASTER
// {
// CFL_fine_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh));
// CFL_coarse_current_passive = SU2_TYPE::GetValue(config->GetCFL(iMesh+1));
// }
// END_SU2_OMP_MASTER
/*--- Implicit barrier here ensures all threads see the CFL values before proceeding ---*/

/*--- Compute adaptive CFL for coarse grid using passive values (no tape recording) ---*/

//nijso: just put a fixed value to see if the omp problem is really here...
passivedouble CFL_coarse_new = 1.0; //computeMultigridCFL(config, solver_coarse, geometry_coarse, iMesh, CFL_fine_passive, CFL_coarse_current_passive);

Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcarruscag ok, it is definitely this CFL_coarse_new that is giving issues, I put it to constant and the thread sanitizer tests are passing now - no more race conditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants