-
Notifications
You must be signed in to change notification settings - Fork 908
[WIP] fix multigrid agglomeration #2375
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
| * \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, |
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.
We need to pass information about all markers on the seed node (number of markers and index of marker)
| case SUB_SOLVER_TYPE::TURB_SST: | ||
| genericSolver = CreateTurbSolver(kindTurbModel, solver, geometry, config, iMGLevel, false); | ||
| metaData.integrationType = INTEGRATION_TYPE::SINGLEGRID; | ||
| //metaData.integrationType = INTEGRATION_TYPE::MULTIGRID; |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 25 days ago
The best way to fix this problem is to remove the commented-out code on line 313: //metaData.integrationType = INTEGRATION_TYPE::MULTIGRID;. This line is commented out without justification, and does not serve as documentation or a helpful note. No other changes are required (such as reinstating the code), since there is already a definitive assignment for metaData.integrationType just above this line, and no surrounding commentary suggests ambiguity.
Edit SU2_CFD/src/solvers/CSolverFactory.cpp by deleting line 313 entirely. No import, method, or variable changes are necessary.
| @@ -310,7 +310,6 @@ | ||
| case SUB_SOLVER_TYPE::TURB_SST: | ||
| genericSolver = CreateTurbSolver(kindTurbModel, solver, geometry, config, iMGLevel, false); | ||
| metaData.integrationType = INTEGRATION_TYPE::SINGLEGRID; | ||
| //metaData.integrationType = INTEGRATION_TYPE::MULTIGRID; | ||
| break; | ||
| case SUB_SOLVER_TYPE::TEMPLATE: | ||
| genericSolver = new CTemplateSolver(geometry, config); |
|
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. |
|
Thank you. 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. |
|
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. |
|
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? |
|
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. |
…ode/su2 into fix_multigrid_agglomeration
| iRKLimit = 1; | ||
| break; | ||
| } | ||
| //return nRKStep; |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
To fix the problem, simply remove the line containing the commented-out code //return nRKStep; in the GetnRKStep method. No reinstatement is necessary, as returning iRKLimit is the intended behavior according to the surrounding logic and comment documentation. No further changes to code functionality or documentation are needed, as this line does not provide meaningful information in comment form.
Edits are required within Common/include/CConfig.hpp, specifically on line 3061, where the commented-out line should be deleted.
| @@ -3058,7 +3058,6 @@ | ||
| iRKLimit = 1; | ||
| break; | ||
| } | ||
| //return nRKStep; | ||
| return iRKLimit; | ||
| } | ||
|
|
| // auto end2 = Suitable_Second_Neighbors.end(); | ||
| // if (find(Suitable_Second_Neighbors.begin(), end2, lPoint) != end2) continue; |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the issue, we should either remove the commented-out code or reinstate it if it's supposed to be active. Given the context (it is a block of commented code with TODO indicating doubt on its necessity, and the main code seems to work without it), the best fix is to remove these commented-out lines, as reinstating could re-enable possibly broken or unnecessary code. This edit should affect only lines 922–939 and 943–945 in Common/src/geometry/CMultiGridGeometry.cpp, which consist of commented-out code relating to "third neighbors". The surrounding headers, comments, and actual code should not be altered. No new imports, definitions, or methods are necessary.
-
Copy modified line R927
| @@ -919,31 +919,12 @@ | ||
|
|
||
| /// TODO: This repeats the process above but I doubt it catches any more points. | ||
|
|
||
| // vector<unsigned long> Third_Neighbor_Points, Third_Origin_Points; | ||
|
|
||
| // for (auto kPoint : Suitable_Second_Neighbors) { | ||
| // for (auto lPoint : fine_grid->nodes->GetPoints(kPoint)) { | ||
| // /*--- Check that the third neighbor does not belong to the first neighbors or the seed ---*/ | ||
|
|
||
| // auto end1 = First_Neighbor_Points.end(); | ||
| // if (find(First_Neighbor_Points.begin(), end1, lPoint) != end1) continue; | ||
|
|
||
| // /*--- Check that the third neighbor does not belong to the second neighbors ---*/ | ||
|
|
||
| // auto end2 = Suitable_Second_Neighbors.end(); | ||
| // if (find(Suitable_Second_Neighbors.begin(), end2, lPoint) != end2) continue; | ||
|
|
||
| // Third_Neighbor_Points.push_back(lPoint); | ||
| // Third_Origin_Points.push_back(kPoint); | ||
| // } | ||
| // } | ||
|
|
||
| /*--- Identify those third neighbors that are repeated (candidate to be added). ---*/ | ||
|
|
||
| // for (auto iNeighbor = 0ul; iNeighbor < Third_Neighbor_Points.size(); iNeighbor++) { | ||
| // for (auto jNeighbor = iNeighbor + 1; jNeighbor < Third_Neighbor_Points.size(); jNeighbor++) { | ||
| // /*--- Repeated third neighbor with different origin ---*/ | ||
|
|
||
|
|
||
| // if ((Third_Neighbor_Points[iNeighbor] == Third_Neighbor_Points[jNeighbor]) && | ||
| // (Third_Origin_Points[iNeighbor] != Third_Origin_Points[jNeighbor])) { | ||
| // Suitable_Indirect_Neighbors.push_back(Third_Neighbor_Points[iNeighbor]); |
| // Third_Neighbor_Points.push_back(lPoint); | ||
| // Third_Origin_Points.push_back(kPoint); | ||
| // } | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
The best way to fix the problem is to remove the commented-out algorithmic code (lines 922–939) in the file Common/src/geometry/CMultiGridGeometry.cpp. This code is clearly legacy or discarded logic, bracketed with a TODO indicating doubt in its utility. Completely removing this block will improve readability and maintainability, ensuring the only active code is compiled and maintained.
The specific lines to delete are 922–939 (inclusive). There is no need to substitute this with additional documentation or formatting, nor are supporting imports or definitions required for the fix.
| @@ -919,25 +919,8 @@ | ||
|
|
||
| /// TODO: This repeats the process above but I doubt it catches any more points. | ||
|
|
||
| // vector<unsigned long> Third_Neighbor_Points, Third_Origin_Points; | ||
|
|
||
| // for (auto kPoint : Suitable_Second_Neighbors) { | ||
| // for (auto lPoint : fine_grid->nodes->GetPoints(kPoint)) { | ||
| // /*--- Check that the third neighbor does not belong to the first neighbors or the seed ---*/ | ||
|
|
||
| // auto end1 = First_Neighbor_Points.end(); | ||
| // if (find(First_Neighbor_Points.begin(), end1, lPoint) != end1) continue; | ||
|
|
||
| // /*--- Check that the third neighbor does not belong to the second neighbors ---*/ | ||
|
|
||
| // auto end2 = Suitable_Second_Neighbors.end(); | ||
| // if (find(Suitable_Second_Neighbors.begin(), end2, lPoint) != end2) continue; | ||
|
|
||
| // Third_Neighbor_Points.push_back(lPoint); | ||
| // Third_Origin_Points.push_back(kPoint); | ||
| // } | ||
| // } | ||
|
|
||
| /*--- Identify those third neighbors that are repeated (candidate to be added). ---*/ | ||
|
|
||
| // for (auto iNeighbor = 0ul; iNeighbor < Third_Neighbor_Points.size(); iNeighbor++) { |
| // for (auto iNeighbor = 0ul; iNeighbor < Third_Neighbor_Points.size(); iNeighbor++) { | ||
| // for (auto jNeighbor = iNeighbor + 1; jNeighbor < Third_Neighbor_Points.size(); jNeighbor++) { | ||
| // /*--- Repeated third neighbor with different origin ---*/ |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To resolve the issue, the best approach is to remove the commented-out block of code starting at line 943 and running through line 952 in Common/src/geometry/CMultiGridGeometry.cpp, as well as any related code that is clearly commented out and not serving documentation purposes. If the functionality is still needed, it should be properly reinstated as live code; otherwise, eliminate the dead code to reduce clutter and potential confusion. No changes to functionality will occur if the block stays commented out, so removal is safe unless evidence suggests it should be active.
| @@ -940,16 +940,7 @@ | ||
|
|
||
| /*--- Identify those third neighbors that are repeated (candidate to be added). ---*/ | ||
|
|
||
| // for (auto iNeighbor = 0ul; iNeighbor < Third_Neighbor_Points.size(); iNeighbor++) { | ||
| // for (auto jNeighbor = iNeighbor + 1; jNeighbor < Third_Neighbor_Points.size(); jNeighbor++) { | ||
| // /*--- Repeated third neighbor with different origin ---*/ | ||
|
|
||
| // if ((Third_Neighbor_Points[iNeighbor] == Third_Neighbor_Points[jNeighbor]) && | ||
| // (Third_Origin_Points[iNeighbor] != Third_Origin_Points[jNeighbor])) { | ||
| // Suitable_Indirect_Neighbors.push_back(Third_Neighbor_Points[iNeighbor]); | ||
| // } | ||
| // } | ||
| // } | ||
|
|
||
| /*--- Remove duplicates from the final list of Suitable Indirect Neighbors. ---*/ | ||
|
|
| // if ((Third_Neighbor_Points[iNeighbor] == Third_Neighbor_Points[jNeighbor]) && | ||
| // (Third_Origin_Points[iNeighbor] != Third_Origin_Points[jNeighbor])) { | ||
| // Suitable_Indirect_Neighbors.push_back(Third_Neighbor_Points[iNeighbor]); | ||
| // } | ||
| // } | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the problem, the commented-out code block should be removed, as it is not currently serving any useful purpose (it is not documentation or example code). This means deleting all lines in the contiguous block of commented-out code, specifically lines 928–959, where code is commented out but otherwise matches active code style and structure.
No additional imports, methods, or definitions are necessary; just delete the commented-out code. Be careful not to remove real comments or documentation, but all evidence suggests these lines are disabled code.
-
Copy modified lines R932-R940 -
Copy modified lines R945-R953 -
Copy modified lines R956-R958
| @@ -925,37 +925,37 @@ | ||
| // for (auto lPoint : fine_grid->nodes->GetPoints(kPoint)) { | ||
| // /*--- Check that the third neighbor does not belong to the first neighbors or the seed ---*/ | ||
|
|
||
| // auto end1 = First_Neighbor_Points.end(); | ||
| // if (find(First_Neighbor_Points.begin(), end1, lPoint) != end1) continue; | ||
|
|
||
| // /*--- Check that the third neighbor does not belong to the second neighbors ---*/ | ||
|
|
||
| // auto end2 = Suitable_Second_Neighbors.end(); | ||
| // if (find(Suitable_Second_Neighbors.begin(), end2, lPoint) != end2) continue; | ||
|
|
||
| // Third_Neighbor_Points.push_back(lPoint); | ||
| // Third_Origin_Points.push_back(kPoint); | ||
| // } | ||
| // } | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| /*--- Identify those third neighbors that are repeated (candidate to be added). ---*/ | ||
|
|
||
| // for (auto iNeighbor = 0ul; iNeighbor < Third_Neighbor_Points.size(); iNeighbor++) { | ||
| // for (auto jNeighbor = iNeighbor + 1; jNeighbor < Third_Neighbor_Points.size(); jNeighbor++) { | ||
| // /*--- Repeated third neighbor with different origin ---*/ | ||
|
|
||
| // if ((Third_Neighbor_Points[iNeighbor] == Third_Neighbor_Points[jNeighbor]) && | ||
| // (Third_Origin_Points[iNeighbor] != Third_Origin_Points[jNeighbor])) { | ||
| // Suitable_Indirect_Neighbors.push_back(Third_Neighbor_Points[iNeighbor]); | ||
| // } | ||
| // } | ||
| // } | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| /*--- Remove duplicates from the final list of Suitable Indirect Neighbors. ---*/ | ||
|
|
||
| // sort(Suitable_Indirect_Neighbors.begin(), Suitable_Indirect_Neighbors.end()); | ||
| // auto it2 = unique(Suitable_Indirect_Neighbors.begin(), Suitable_Indirect_Neighbors.end()); | ||
| // Suitable_Indirect_Neighbors.resize(it2 - Suitable_Indirect_Neighbors.begin()); | ||
|
|
||
|
|
||
|
|
||
| } | ||
|
|
||
| void CMultiGridGeometry::AgglomerateImplicitLines(unsigned long& Index_CoarseCV, const CGeometry* fine_grid, |
| // sort(Suitable_Indirect_Neighbors.begin(), Suitable_Indirect_Neighbors.end()); | ||
| // auto it2 = unique(Suitable_Indirect_Neighbors.begin(), Suitable_Indirect_Neighbors.end()); | ||
| // Suitable_Indirect_Neighbors.resize(it2 - Suitable_Indirect_Neighbors.begin()); |
Check notice
Code scanning / CodeQL
Commented-out code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
The best way to fix this problem is to remove the commented-out code at lines 956-958, since the deduplication logic is not active and its purpose is unclear. Removing these lines will clean up the surrounding code and avoid distraction/confusion for future maintainers. No other code or functionality should be modified; this fix only involves deleting lines 956-958.
-
Copy modified line R956
| @@ -953,9 +953,7 @@ | ||
|
|
||
| /*--- Remove duplicates from the final list of Suitable Indirect Neighbors. ---*/ | ||
|
|
||
| // sort(Suitable_Indirect_Neighbors.begin(), Suitable_Indirect_Neighbors.end()); | ||
| // auto it2 = unique(Suitable_Indirect_Neighbors.begin(), Suitable_Indirect_Neighbors.end()); | ||
| // Suitable_Indirect_Neighbors.resize(it2 - Suitable_Indirect_Neighbors.begin()); | ||
|
|
||
| } | ||
|
|
||
| void CMultiGridGeometry::AgglomerateImplicitLines(unsigned long& Index_CoarseCV, const CGeometry* fine_grid, |
| unsigned short iLimit = config[iZone]->GetnRKStep(); | ||
|
|
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.
all that stuff was not used.
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.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

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:
current features:
TO DO:
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.
pre-commit run --allto format old commits.