-
Notifications
You must be signed in to change notification settings - Fork 917
[WIP] Fix multigrid agglomeration #2712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
|
||
| const unsigned short nVar = sol_fine->GetnVar(); | ||
| const su2double factor = config->GetDamp_Correc_Prolong(); | ||
| //const su2double factor = config->GetDamp_Correc_Prolong(); |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 hour ago
In general, to fix commented‑out code, either remove it entirely or reinstate it as active code, and rely on version control history if you ever need to recover old implementations. If the intent is to document behavior, rewrite it as an explanatory comment rather than a compilable snippet.
For this case, the best fix without changing existing functionality is to delete the commented‑out declaration of factor on line 649. The active implementation already sets factor via base_damp, so removing the old, commented line will not alter behavior. No new methods, imports, or definitions are required; only the single line within CMultiGridIntegration::SetProlongated_Correction in SU2_CFD/src/integration/CMultiGridIntegration.cpp needs to be removed.
| @@ -646,7 +646,6 @@ | ||
| su2double *Solution_Fine, *Residual_Fine; | ||
|
|
||
| const unsigned short nVar = sol_fine->GetnVar(); | ||
| //const su2double factor = config->GetDamp_Correc_Prolong(); | ||
|
|
||
| /*--- Apply level-dependent damping: more aggressive damping on deeper coarse levels ---*/ | ||
| const su2double base_damp = config->GetDamp_Correc_Prolong(); |
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When MG is finalized, these will be initial values and will be adaptively changed.
| /*--- Note: CFL at the coarse levels have a large impact on convergence, | ||
| this should be rewritten to use adaptive CFL. ---*/ | ||
| const su2double factor = 1.0; | ||
| const su2double Coeff = 1.1; //pow(su2double(Global_nPointFine) / Global_nPointCoarse, 1.0 / nDim); |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 hour ago
To fix the problem, we should eliminate the commented-out code while keeping the current behavior (Coeff equal to 1.1) unchanged. Since the comment above already hints that the CFL strategy “should be rewritten to use adaptive CFL,” we can either (a) delete the commented pow(...) call entirely, or (b) turn it into a clearer descriptive comment that does not look like disabled code. The minimal, safest change is to remove the commented pow(...) expression, leaving the constant assignment as-is.
Concretely, in Common/src/geometry/CMultiGridGeometry.cpp, in the block around lines 718–724, we will modify the line
722: const su2double Coeff = 1.1; //pow(su2double(Global_nPointFine) / Global_nPointCoarse, 1.0 / nDim);to remove the commented-out code. We can either leave just the numeric assignment or replace the trailing comment with a short explanation that does not contain code-like syntax. For clarity and to retain design intent, we can change it to:
const su2double Coeff = 1.1; // Fixed coefficient; see documentation for CFL strategy.This preserves existing functionality while eliminating the commented-out code pattern that CodeQL flags.
-
Copy modified line R722
| @@ -719,7 +719,7 @@ | ||
| /*--- Note: CFL at the coarse levels have a large impact on convergence, | ||
| this should be rewritten to use adaptive CFL. ---*/ | ||
| const su2double factor = 1.0; | ||
| const su2double Coeff = 1.1; //pow(su2double(Global_nPointFine) / Global_nPointCoarse, 1.0 / nDim); | ||
| const su2double Coeff = 1.1; // Fixed coefficient used in current CFL strategy. | ||
| const su2double CFL = factor * config->GetCFL(iMesh - 1) / Coeff; | ||
| config->SetCFL(iMesh, CFL); | ||
| } |
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 --allto 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.