Skip to content

Comments

Fix multigrid agglomeration#2712

Open
bigfooted wants to merge 77 commits intodevelopfrom
fix_MG_part_1
Open

Fix multigrid agglomeration#2712
bigfooted wants to merge 77 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.

bigfooted and others added 12 commits February 3, 2026 19:37
Co-authored-by: Johannes Blühdorn <55186095+jblueh@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Comment on lines +563 to +565
// nijso: check if this can be removed now.
//solver_fine->InitiateComms(geometry_fine, config, MPI_QUANTITIES::SOLUTION);
//solver_fine->CompleteComms(geometry_fine, config, MPI_QUANTITIES::SOLUTION);

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.

Copilot Autofix

AI 1 day ago

To fix the problem, remove the commented-out code lines so that no executable statements are kept as comments. This eliminates confusion and ensures the codebase reflects only active logic.

Concretely, in SU2_CFD/src/integration/CMultiGridIntegration.cpp, inside CMultiGridIntegration::MultiGrid_Cycle, in the post-smoothing loop near line 544, delete the two lines that comment out solver_fine->InitiateComms(...) and solver_fine->CompleteComms(...). Keep the explanatory comment on line 545 if you still want a note about potential synchronization considerations, or adjust its wording; this change does not alter any executable behavior because those calls are already disabled.

No new methods, imports, or definitions are needed; we are only removing 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
@@ -543,8 +543,6 @@
 
       /*--- MPI sync after RK stage to ensure halos have updated solution for next smoothing iteration ---*/
       // nijso: check if this can be removed now.
-      //solver_fine->InitiateComms(geometry_fine, config, MPI_QUANTITIES::SOLUTION);
-      //solver_fine->CompleteComms(geometry_fine, config, MPI_QUANTITIES::SOLUTION);
 
     }
   }
EOF
@@ -543,8 +543,6 @@

/*--- MPI sync after RK stage to ensure halos have updated solution for next smoothing iteration ---*/
// nijso: check if this can be removed now.
//solver_fine->InitiateComms(geometry_fine, config, MPI_QUANTITIES::SOLUTION);
//solver_fine->CompleteComms(geometry_fine, config, MPI_QUANTITIES::SOLUTION);

}
}
Copilot is powered by AI and may make mistakes. Always verify output.
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.

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