-
Notifications
You must be signed in to change notification settings - Fork 905
[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 😄 |
Proposed Changes
This draft PR introduces a nested Krylov solver setup in SU2.
The main contribution is enabling FGMRES to use BiCGSTAB as an inner preconditioner.
The aim is to improve convergence robustness and efficiency for stiff linear systems, especially in high-Re/low-Mach cases.
Changes include:
Modifications to the linear solver routines (CSysSolve.cpp, CSysSolve.hpp), CConfig.cpp and option_structure.hpp
Added new functions to allow BiCGSTAB calls inside FGMRES iterations.
Extended configuration parsing to accept LINEAR_SOLVER= FGMRESandBCGSTAB2
Related Work
Builds upon existing Krylov-based solvers already implemented in SU2 (FGMRES, BiCGSTAB).
Complements discussions on improving solver performance for stiff CFD problems.
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.