Skip to content

Use modified Gram-Schmidt orthogonalization in GMRES - #600

Draft
michelebucelli wants to merge 5 commits into
SimVascular:mainfrom
michelebucelli:fix/modified-gram-schmidt-gmres
Draft

Use modified Gram-Schmidt orthogonalization in GMRES#600
michelebucelli wants to merge 5 commits into
SimVascular:mainfrom
michelebucelli:fix/modified-gram-schmidt-gmres

Conversation

@michelebucelli

Copy link
Copy Markdown
Collaborator

Fixes #599.

Current situation

The GMRES method, as implemented in gmres.cpp, used unmodified Gram-Schmidt orthogonalization. Multiple experiments indicate that the numerical instability related to that propagates to other parts of the solver. See #599 for additional details.

Release Notes

  1. GMRES uses modified Gram-Schmidt orthogonalization;
  2. the orthogonalization step has been extracted to two dedicated functions (one for scalar and one for vector problems), for improved readability and to remove duplication between the two vector versions of GMRES (gmres and gmres_v);
  3. Addressed a @todo about replacing value = value + ... with value += ... in FourierInterpolation.

Documentation

The new functions that wrap the orthogonalization step have Doxygen documentation.

None of these changes is user-facing.

Testing

All automatic tests are still passing.

Code of Conduct & Contributing Guidelines

@michelebucelli
michelebucelli requested review from aabrown100-git, javijv4 and ktbolt and a lite review from Copilot August 13, 2026 19:55

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@michelebucelli michelebucelli changed the title Fix/modified gram schmidt gmres Use modified Gram-Schmidt orthogonalization in GMRES Aug 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request updates the GMRES linear solver to use modified Gram–Schmidt (MGS) orthogonalization (extracted into dedicated helper functions) to improve numerical stability and reduce divergence between MPI runs, and also performs a small cleanup in Fourier interpolation evaluation.

Changes:

  • Replaced classical Gram–Schmidt with modified Gram–Schmidt in GMRES and refactored the orthogonalization step into shared helper functions.
  • Added a “lucky breakdown” handling path based on comparing the subdiagonal Hessenberg entry to machine epsilon-scaled norms.
  • Simplified arithmetic in FourierInterpolation by using compound assignment operators (-= / +=).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
Code/Source/linear_solver/gmres.cpp Introduces MGS orthogonalization helpers and replaces the previous orthogonalization logic in GMRES variants.
Code/Source/solver/FourierInterpolation.cpp Uses compound assignment operators for trend subtraction and Fourier series accumulation.
Suppressed comments (2)

Code/Source/linear_solver/gmres.cpp:426

  • The new lucky-breakdown handling in orthogonalize_* can set h(i+1,i)=0.0, which makes the subsequent Givens-rotation normalization tmp = sqrt(h(i,i)^2 + h(i+1,i)^2) potentially become 0.0. That leads to division by zero when computing c(i) and s(i) and can produce NaNs.
      orthogonalize_s(lhs, nNo, mynNo, i, u, h);

Code/Source/linear_solver/gmres.cpp:598

  • The new lucky-breakdown handling in orthogonalize_* can set h(i+1,i)=0.0, which makes the subsequent Givens-rotation normalization tmp = sqrt(h(i,i)^2 + h(i+1,i)^2) potentially become 0.0. That leads to division by zero when computing c(i) and s(i) and can produce NaNs.
      orthogonalize_v(lhs, dof, nNo, mynNo, i, u, h);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Code/Source/linear_solver/gmres.cpp
Comment thread Code/Source/linear_solver/gmres.cpp
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.71%. Comparing base (a3413e0) to head (d3c66cc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #600      +/-   ##
==========================================
- Coverage   72.73%   72.71%   -0.02%     
==========================================
  Files         255      255              
  Lines       39290    39273      -17     
  Branches     6726     6725       -1     
==========================================
- Hits        28576    28557      -19     
- Misses      10472    10473       +1     
- Partials      242      243       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aabrown100-git aabrown100-git left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, pending resolution of copilot's comments

@ktbolt

ktbolt commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

I would like to see some tests on real problems to collect data on performance changes, improved convergence, etc.

@michelebucelli

Copy link
Copy Markdown
Collaborator Author

Moving this to draft, pending the discussion at #599.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Electromechanics results diverge in parallel when using GMRES

4 participants