Support left preconditioning in GMRES by kakeueda · Pull Request #417 · ORNL/ReSolve

kakeueda · 2026-03-04T21:32:36Z

Description

This change allows users to solve a left-preconditioned system $M^{-1}Ax = M^{-1}b$ using GMRES. It also adds a PreconditionerUserMatrix class.

@JeffZ594 My understanding is that we eventually want to add regularization to the GMRES solvers. We can follow up with another PR that adds regularization support with testTripsProblem.

Proposed changes

Updated GMRES to support left preconditioning.
Added a PreconditionerUserMatrix class so users can define $M^{-1}$ explicitly.
Added unit tests for lu and usermatrix preconditioners.
Added a new command line option -p left (or right) for the GMRES solver (default: right).

Checklist

Further comments

Left preconditioning for FGMRES is more involved, so for now non-flexible GMRES only. If FGMRES is selected with -p left, the solver reports an error.

Copilot

Pull request overview

This PR extends ReSolve’s GMRES implementations to support left preconditioning (M^{-1}Ax = M^{-1}b), adds a user-supplied explicit preconditioner matrix via PreconditionerUserMatrix, and wires the feature through examples/tests and CLI (-p left|right).

Changes:

Add Preconditioner::getSide()/setSide() and plumb side selection through SystemSolver::preconditionerSetup(...).
Implement left-preconditioned variants of the GMRES/Randomized GMRES iteration logic (with fallback to right preconditioning for flexible GMRES).
Add unit/functionality tests and example updates for left preconditioning and the new user-matrix preconditioner.

Reviewed changes

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

Show a summary per file

File	Description
`resolve/Preconditioner.hpp`	Adds preconditioning side API (`left`/`right`).
`resolve/Preconditioner.cpp`	Implements `getSide()` / `setSide()`.
`resolve/SystemSolver.hpp`	Extends preconditioner setup signature and exposes `getPreconditioner()`.
`resolve/SystemSolver.cpp`	Applies preconditioner setup with chosen side; exposes preconditioner getter.
`resolve/PreconditionerUserMatrix.hpp`	Declares user-provided explicit preconditioner matrix wrapper.
`resolve/PreconditionerUserMatrix.cpp`	Implements user-matrix preconditioner via `MatrixHandler::matvec`.
`resolve/LinSolverIterativeFGMRES.hpp/.cpp`	Implements left-preconditioning behavior (non-flexible GMRES) and warns/falls back for flexible GMRES.
`resolve/LinSolverIterativeRandFGMRES.hpp/.cpp`	Implements left-preconditioning behavior in randomized GMRES variant.
`resolve/PreconditionerLU.hpp`	Updates file header comment.
`resolve/PreconditionerLU.cpp`	Updates documentation strings/comments for LU preconditioner.
`resolve/CMakeLists.txt`	Adds new preconditioner source/header to build/install.
`tests/unit/preconditioner/*`	Adds LU and user-matrix preconditioner unit tests and CMake integration.
`tests/unit/CMakeLists.txt`	Adds preconditioner unit test subdirectory.
`tests/functionality/testSysGmres.cpp`	Adds `-p` parsing and conditional residual checking for left preconditioning.
`tests/functionality/CMakeLists.txt`	Adds functionality tests covering left-preconditioned runs.
`examples/sysGmres.cpp`	Adds `-p` parsing and passes side into `preconditionerSetup`.
`examples/randGmres.cpp`	Refactors preconditioner setup to use `PreconditionerLU` consistently.
`CHANGELOG.md`	Adds entry documenting left preconditioning + user-matrix preconditioner.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

resolve/LinSolverIterativeRandFGMRES.cpp

tests/functionality/testSysGmres.cpp

resolve/PreconditionerLU.hpp

resolve/Preconditioner.hpp

Copilot · 2026-03-04T22:08:10Z

resolve/PreconditionerUserMatrix.cpp

+    matrix_handler_->matvec(B_, rhs, x, &ONE, &ZERO, memspace_);
+    return 0;
+  }
+
+  void PreconditionerUserMatrix::setMemorySpace()
+  {
+    bool is_matrix_handler_cuda = matrix_handler_->getIsCudaEnabled();
+    bool is_matrix_handler_hip  = matrix_handler_->getIsHipEnabled();
+


PreconditionerUserMatrix::apply() unconditionally calls MatrixHandler::matvec() and then returns 0. If B_ or matrix_handler_ is null this will crash, and any nonzero status from matvec is currently ignored. Please add null checks (e.g., require setPrecMatrix() to have been called) and propagate/return the matvec status so callers can detect failures.

Suggested change

matrix_handler_->matvec(B_, rhs, x, &ONE, &ZERO, memspace_);

return 0;

}

void PreconditionerUserMatrix::setMemorySpace()

{

bool is_matrix_handler_cuda = matrix_handler_->getIsCudaEnabled();

bool is_matrix_handler_hip = matrix_handler_->getIsHipEnabled();

// Ensure that the matrix handler and preconditioner matrix have been set

if (matrix_handler_ == nullptr || B_ == nullptr)

{

// Nonzero return value indicates failure (e.g., missing setup)

return 1;

}

int status = matrix_handler_->matvec(B_, rhs, x, &ONE, &ZERO, memspace_);

return status;

}

void PreconditionerUserMatrix::setMemorySpace()

{

bool is_matrix_handler_cuda = false;

bool is_matrix_handler_hip = false;

if (matrix_handler_ != nullptr)

{

is_matrix_handler_cuda = matrix_handler_->getIsCudaEnabled();

is_matrix_handler_hip = matrix_handler_->getIsHipEnabled();

}

resolve/PreconditionerUserMatrix.cpp

resolve/SystemSolver.cpp

CHANGELOG.md

JeffZ594 · 2026-03-09T17:29:47Z

@kakeueda

Thank you for making the unit tests for the preconditioners! I have code that implements regularization in Eigen that needs to be translated that would be better for testTripsProblem. I think these tests work better for testing just the preconditioners.

kakeueda · 2026-03-10T20:24:13Z

@kakeueda

Thank you for making the unit tests for the preconditioners! I have code that implements regularization in Eigen that needs to be translated that would be better for testTripsProblem. I think these tests work better for testing just the preconditioners.

Sure, let me first finish this PR and I'll take a look at your implementation .
@pelesh Are you okay with adding Eigen as a new dependency?

JeffZ594 · 2026-03-10T20:27:43Z

@kakeueda I believe Dr Peles does not want to use Eigen as a dependency. Right now I only use Eigen to calculate the SVD of the Hessenberg Matrix for GMRES but I believe there are alternatives like LAPACK that can also do this.

pelesh · 2026-03-18T22:00:57Z

@kakeueda I believe Dr Peles does not want to use Eigen as a dependency. Right now I only use Eigen to calculate the SVD of the Hessenberg Matrix for GMRES but I believe there are alternatives like LAPACK that can also do this.

Hessenberg matrix is dense, so SVD from LAPACK should work in this case. I wouldn't add Eigen as a dependency unless there is compelling enough reason to do so.

Co-authored-by: Kakeru Ueda <kakecoco2580@icloud.com>

Fixed some small bugs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

kakeueda · 2026-03-19T13:24:02Z

For now the default behavior for left preconditioning is to use the preconditioned residual $||M^{-1}(b - Ax)||$ for convergence check and the true residual $||(b - Ax)||$ for monitoring (final_residual_norm_ stores this).

kakeueda · 2026-03-19T13:30:37Z

This is outside the scope of this PR but 'solve()' in GMRES is starting to get a bit cluttered. It might be worth considering adding a base GMRES class and having the current RandFGMRES and regularized GMRES derive from it. We could also clean up the convergence check for GMRES which I believe this was mentioned before.

resolve/LinSolverIterativeFGMRES.cpp

resolve/LinSolverIterativeRandFGMRES.cpp

shakedregev · 2026-03-19T14:35:45Z

resolve/LinSolverIterativeRandFGMRES.cpp

+        true_rnorm = vector_handler_->dot(vec_V_, vec_V_, memspace_);
+        true_rnorm = std::sqrt(true_rnorm);


maybe we should change this to true_res_norm? r can be anything, it's not clear from context (and there's no separator from norm)

Agree, also changed bnorm to rhs_norm

shakedregev · 2026-03-19T14:43:34Z

tests/unit/preconditioner/PreconditionerUserMatrixTests.hpp

+      /**
+       * @brief Test apply() performs correct matvec with B.
+       *
+       * B = diag(2, 3, 4), x = [1, 1, 1]^T => y = [2, 3, 4]^T.


Why tranpose?

It was meant to show a column vector, but maybe unnecessary

Worse, it's misleading. Vectors are by default column, unless you put a transpose. That's what didn't make sense to me. I knew it had to be a column, but this is the convention for row.

Co-authored-by: Shaked Regev <35384901+shakedregev@users.noreply.github.com>

shakedregev

Some minor comments. I need to test and I will change this to approve.

shakedregev

Great work.

kakeueda · 2026-03-19T17:20:11Z

returned 503: Egress is over the account limit.

I got this error in the test. Any idea what might be causing this? @pelesh

nkoukpaizan · 2026-03-19T17:42:43Z

returned 503: Egress is over the account limit.

I got this error in the test. Any idea what might be causing this? @pelesh

Looks like a temporary failure in GH-Actions. It's working now!

Copilot AI review requested due to automatic review settings March 4, 2026 21:32

kakeueda self-assigned this Mar 4, 2026

Copilot started reviewing on behalf of kakeueda March 4, 2026 21:33 View session

kakeueda requested review from Copilot and removed request for Copilot March 4, 2026 21:35

Copilot started reviewing on behalf of kakeueda March 4, 2026 22:00 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

kakeueda marked this pull request as draft March 4, 2026 23:52

JeffZ594 and others added 18 commits March 19, 2026 15:06

ABBA GMRES Commit

f469ac5

Added check for preconditioner being set

d7716e2

Use Preconditioner Class for ABBAGMRES

7ab3e29

Apply pre-commmit fixes

df69725

Apply pre-commmit fixes

d8c6f39

Update resolve/PreconditionerMatvec.cpp

0104791

Co-authored-by: Kakeru Ueda <kakecoco2580@icloud.com>

Update resolve/LinSolverIterativeFGMRES.cpp

1032c26

Co-authored-by: Kakeru Ueda <kakecoco2580@icloud.com>

Update resolve/Preconditioner.hpp

d2a590e

Co-authored-by: Kakeru Ueda <kakecoco2580@icloud.com>

Apply pre-commmit fixes

4bb1fac

Renamed PreconditionerMatvec to PreconditionerABBA

0588909

Fixed some small bugs

Apply pre-commmit fixes

8c899bd

Fix left preconditioning and add corresponding tests

e620de1

Fix typo and some comments

8e55d7d

Apply precommit and update CHANGELOG.md

2714e54

Update CHANGELOG.md

b0f3ebb

Use preconditioned bnorm in RandFGMRES

8be5465

Apply pre-commmit fixes

68807d0

Remove unnecessary variables

f34279f

kakeueda and others added 3 commits March 19, 2026 15:06

Apply pre-commmit fixes

25d85e8

[skip ci] Update CHANGELOG.md

ca953b9

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Fix norm evaluation in GMRES

4d36a83

kakeueda force-pushed the kakeru/preconditioner-ABBA branch from e866f59 to 4d36a83 Compare March 19, 2026 06:09

Fix naming and error check

03175f7

ORNL deleted a comment from Copilot AI Mar 19, 2026

kakeueda added 2 commits March 19, 2026 21:45

[skip ci] Move #pragma above

1c28d2e

[skip ci] Fix typo

4238c7f

kakeueda marked this pull request as ready for review March 19, 2026 13:30

kakeueda requested review from pelesh and shakedregev March 19, 2026 13:31

shakedregev reviewed Mar 19, 2026

View reviewed changes

resolve/LinSolverIterativeFGMRES.cpp Outdated Show resolved Hide resolved

shakedregev reviewed Mar 19, 2026

View reviewed changes

resolve/LinSolverIterativeRandFGMRES.cpp Outdated Show resolved Hide resolved

shakedregev reviewed Mar 19, 2026

View reviewed changes

Apply suggestions from code review

258c058

Co-authored-by: Shaked Regev <35384901+shakedregev@users.noreply.github.com>

shakedregev reviewed Mar 19, 2026

View reviewed changes

shakedregev approved these changes Mar 19, 2026

View reviewed changes

Fix naming and comment

581a946

-    matrix_handler_->matvec(B_, rhs, x, &ONE, &ZERO, memspace_);
-    return 0;
-  }
-  void PreconditionerUserMatrix::setMemorySpace()
-  {
-    bool is_matrix_handler_cuda = matrix_handler_->getIsCudaEnabled();
-    bool is_matrix_handler_hip  = matrix_handler_->getIsHipEnabled();
+    // Ensure that the matrix handler and preconditioner matrix have been set
+    if (matrix_handler_ == nullptr || B_ == nullptr)
+    {
+      // Nonzero return value indicates failure (e.g., missing setup)
+      return 1;
+    }
+    int status = matrix_handler_->matvec(B_, rhs, x, &ONE, &ZERO, memspace_);
+    return status;
+  }
+  void PreconditionerUserMatrix::setMemorySpace()
+  {
+    bool is_matrix_handler_cuda = false;
+    bool is_matrix_handler_hip  = false;
+    if (matrix_handler_ != nullptr)
+    {
+      is_matrix_handler_cuda = matrix_handler_->getIsCudaEnabled();
+      is_matrix_handler_hip  = matrix_handler_->getIsHipEnabled();
+    }

		true_rnorm = vector_handler_->dot(vec_V_, vec_V_, memspace_);
		true_rnorm = std::sqrt(true_rnorm);

Conversation

kakeueda commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Proposed changes

Checklist

Further comments

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeffZ594 commented Mar 9, 2026

Uh oh!

kakeueda commented Mar 10, 2026

Uh oh!

JeffZ594 commented Mar 10, 2026

Uh oh!

pelesh commented Mar 18, 2026

Uh oh!

kakeueda commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kakeueda commented Mar 19, 2026

Uh oh!

Uh oh!

Uh oh!

shakedregev Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

kakeueda Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

shakedregev Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

kakeueda Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

shakedregev Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

shakedregev left a comment

Choose a reason for hiding this comment

Uh oh!

shakedregev left a comment

Choose a reason for hiding this comment

Uh oh!

kakeueda commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkoukpaizan commented Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kakeueda commented Mar 4, 2026 •

edited

Loading

kakeueda commented Mar 19, 2026 •

edited

Loading

kakeueda commented Mar 19, 2026 •

edited

Loading