Skip to content

Conversation

@eorbay-metu
Copy link
Member

@eorbay-metu eorbay-metu commented Sep 8, 2025

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.

  • Extend CSysSolve so that FGMRES can call an inner BiCGSTAB solve instead of a single preconditioner application, when enabled in the config.
  • Introduce a small helper,
    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.

  • 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.

@eorbay-metu eorbay-metu force-pushed the feature_precond_linsol branch from d11078e to 0d8e9fb Compare September 8, 2025 09:29
@pcarruscag pcarruscag changed the base branch from master to develop September 8, 2025 14:23
@pcarruscag pcarruscag changed the title Add nested preconditioner: FGMRES with BiCGSTAB [WIP] Add nested preconditioner: FGMRES with BiCGSTAB Sep 8, 2025
Copy link
Member

@pcarruscag pcarruscag left a 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.

@eorbay-metu
Copy link
Member Author

@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.

@bigfooted
Copy link
Contributor

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.

@bigfooted
Copy link
Contributor

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)

Copy link
Contributor

@github-advanced-security github-advanced-security bot left a 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.

@eorbay-metu
Copy link
Member Author

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!

@bigfooted
Copy link
Contributor

bigfooted commented Nov 18, 2025

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.

@pcarruscag
Copy link
Member

If the plan is to integrate this PR in the develop branch, my initial comment needs to be addressed.
I would also like to see some results on the effectiveness of this nested preconditioning.

@bigfooted
Copy link
Contributor

SU2 conference presentation:
https://drive.google.com/file/d/1jua_BXhRtSlb437Ydg_4pI76bHKaRrZ6/view

Next week on Wednesday at 16:00 CET we have a developer meeting at https://meet.jit.si/SU2_DevMeeting

@pcarruscag
Copy link
Member

Yes, I know, it would have to be more compelling than that to get me to ok copy-pasting code 😄

@bigfooted
Copy link
Contributor

bigfooted commented Nov 19, 2025

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. :-)

@eorbay-metu
Copy link
Member Author

Yes, I know, it would have to be more compelling than that to get me to ok copy-pasting code 😄

Haha, fair enough 😄
Thanks for pointing it out — I'll revise accordingly shortly.
Hopefully this time it will be worthy of passing the gate-keeper’s check!

Copy link
Member

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

Copy link
Member Author

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.

Comment on lines +178 to +180
void BCGSTABpre_parallel(const CSysVector<ScalarType>& a, CSysVector<ScalarType>& b_in,
const CMatrixVectorProduct<ScalarType>& mat_vec, const CPreconditioner<ScalarType>& precond,
const CConfig* config) {
Copy link
Member

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?

Copy link
Member Author

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 = YES
  • LINEAR_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.

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