-
Notifications
You must be signed in to change notification settings - Fork 910
[WIP] Add nested preconditioner: FGMRES with BiCGSTAB #2566
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
d11078e to
0d8e9fb
Compare
pcarruscag
left a comment
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.
Hi, the nice of implementing this feature is to wrap the inner linear solver into the generic preconditioner operation that is passed to FGMRES (or any other solver).
Please give it some thought, we can talk more in one of the weekly developer meetings.
|
@pcarruscag Thank you for the suggestion! I understand your point about wrapping the inner solver into the generic preconditioner interface for broader reuse. In my initial implementation I integrated BiCGSTAB directly inside FGMRES rather than as a separate preconditioner function, because my primary goal was to quickly evaluate the nested Krylov approach in SU2 and confirm its effect on convergence. This way I could test the functionality without restructuring the existing preconditioner hierarchy. I see the benefit of a cleaner design by wrapping it into the preconditioner interface, and I will look into how to move in that direction. I would be glad to discuss this further during one of the weekly developer meetings. |
|
Hi, great work, can you also add a small testcase that uses the new function? Looking forward to hearing more about it at the conference. |
|
Hi @eorbay-metu , What needs to be done for the PR to get out of draft mode? Can you add a testcase that uses the new FGMRES/BiCGSTAB preconditioner (modify an existing testcase) |
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.
|
Hi @bigfooted , thanks for checking in! I'm currently working through the remaining CI issues. The compilation and regression tests are green, but the Code Style workflow is still reporting formatting errors. I’m planning to run clang-format/pre‑commit locally and push a cleaned-up commit to address those shortly. To show that the new nested FGMRES/BiCGSTAB preconditioner works, I’ve prepared a modified version of an existing RANS/NACA test that uses this nested solver. Once the formatting is fixed and the new test case is integrated, I’ll take the PR out of draft and open it up for full review. Thanks for your patience and feedback — if you have a particular scenario you’d like me to cover, please let me know! |
|
Thanks, looks like your commit fixed it, and do not worry about the codefactor issues. For the regression tests, we would just need a case that uses your implementation, so a simple naca0012 airfoil that we already have (or if you have a simple alternative) can be used. It should run fast, and just 10 iterations or so. If you also have a testcase that can be used in the Verification & Validation tests or as a tutorial, that would be great as well. Just use what you have at the moment. |
|
If the plan is to integrate this PR in the develop branch, my initial comment needs to be addressed. |
|
SU2 conference presentation: Next week on Wednesday at 16:00 CET we have a developer meeting at https://meet.jit.si/SU2_DevMeeting |
|
Yes, I know, it would have to be more compelling than that to get me to ok copy-pasting code 😄 |
thus spoke the gate-keeper. :-) |
Haha, fair enough 😄 |
TestCases/TestCases/.travis.yml
Outdated
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.
I think you copy pasted TestCases into TestCases and now there are a lot of new files
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.
Oh, you’re right – that was a mistake on my side. I accidentally copied the TestCases directory into TestCases/, which created a duplicated tree and a lot of unrelated files in this PR.
I’ll clean this up and. I’ll push the updated version accordingly.
| void BCGSTABpre_parallel(const CSysVector<ScalarType>& a, CSysVector<ScalarType>& b_in, | ||
| const CMatrixVectorProduct<ScalarType>& mat_vec, const CPreconditioner<ScalarType>& precond, | ||
| const CConfig* config) { |
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.
So is this the final version you propose?
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.
Yes, this is the version I am proposing at this stage.
The idea is to keep the inner solver logic inside CSysSolve.cpp and reuse the existing BCGSTAB implementation as an inner linear solver. In the current design, FGMRES behaves as before unless the config explicitly enables nesting, i.e.
LINEAR_SOLVER_NESTED = YESLINEAR_SOLVER_INNER = BCGSTAB
In that case, FGMRES calls the inner BCGSTAB helper instead of the usual preconditioner application. I kept the inner solver out of CPreconditioner.hpp on purpose, to avoid mixing “preconditioner” and “inner Krylov solver” concepts and to prevent confusion since the inner solver itself uses the existing preconditioner infrastructure. There is already a BCGSTAB_LinSolver in CSysSolve which returns an unsigned long. For the nested case, I introduced BCGSTABpre_parallel as a void helper, because I wanted a simpler interface that only takes (A, b, mat_vec, precond, config) and updates x in-place, without changing the existing outer solver interfaces or return types. The nested FGMRES code only needs the updated solution vector, so the helper keeps the call site straightforward. If you prefer, I can refactor this further to reduce duplication.
Right now, the inner solver uses a fixed tolerance (1e-5) hard-coded in the implementation; there are no separate config options yet for the inner solver (e.g. inner tolerance / inner max iterations). My plan is to expose these as dedicated config options in a follow-up patch, once the overall nested FGMRES + BiCGSTAB structure is agreed upon.
Proposed Changes
This PR introduces a nested Krylov solver setup in SU2, where FGMRES can optionally use BiCGSTAB as an inner Krylov solver. The main goal is to improve robustness (and potentially efficiency) for stiff linear systems, especially in challenging high-Re / low-Mach flow cases.
CSysSolveso that FGMRES can call an inner BiCGSTAB solve instead of a single preconditioner application, when enabled in the config.CSysSolve.cpp:
void BCGSTABpre_parallel(const CSysVector& a,
CSysVector& b_in,
const CMatrixVectorProduct& mat_vec,
const CPreconditioner& precond,
const CConfig* config);
which:
--reuses the existing matrix–vector product and preconditioner infrastructure,
--updates the solution vector in-place for the nested FGMRES iteration,
--keeps the existing BCGSTAB_LinSolver API unchanged.
FGMRES behaves as before unless nesting is explicitly turned on.
##Changes include:
The related configuration flags are added to control the nested solver behaviour.
The nested mode is activated by:
LINEAR_SOLVER = FGMRES
LINEAR_SOLVER_NESTED = YES
LINEAR_SOLVER_INNER = BCGSTAB
Internally, a new enum and option mapping are added in CConfig / option_structure.hpp to represent the inner solver kind (INNER_SOLVER_NONE, INNER_SOLVER_BCGSTAB, …).
Update the solver output so that the choice of nested vs. non-nested FGMRES is clearly reported:
Nested FGMRES (FGMRES with inner BiCGSTAB) is used for solving the linear system.
In this PR, the inner BiCGSTAB solver uses fixed tolerance (hard-coded 1e-5 for the inner tolerance). There are no separate config options yet for inner tolerance / inner maximum iterations. A follow-up change can introduce dedicated config options such as:
INNER_LINEAR_SOLVER_ERROR
INNER_LINEAR_SOLVER_ITER
Related Work
Builds on the existing Krylov-based linear solvers (FGMRES, BCGSTAB) already implemented in SU2.
Targets scenarios where the standard preconditioned FGMRES may struggle (stiff, high-Re/low-Mach RANS cases), by allowing a nested Krylov approach without changing the behaviour for users who keep LINEAR_SOLVER_NESTED = NO.
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.