Skip to content

Conversation

@ASunil-17
Copy link
Collaborator

@ASunil-17 ASunil-17 commented Oct 24, 2025

Description

Please describe the issue that is addressed (bug, new feature,
documentation, enhancement, etc.). Please also include relevant motivation and
context. List any dependencies that are required for this change.

Closes # (issue)

Mentions @(user)

Proposed changes

Describe how your changes here address the issue and why the proposed changes
should be accepted.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here
to help! This is simply a reminder of what we are going to look for before
merging your code.

  • All tests pass. Code tested on
    • CPU backend
    • CUDA backend
    • HIP backend
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows Re::Solve style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.
  • I have updated CHANGELOG.md to reflect the changes in this PR. If this is a minor PR that is part of a larger fix already included in the file, state so.

Further comments

Changes made to GramSchmidt.cpp was done for debugging purposes. Will restore back to the original. Changes made to LinSolverIterativeFGMRES.cpp include -

  1. Computing an update vec_Y_ to Ay = r (residual equation) only when initial residual < norm of rhs. Needed to allocate space on DEVICE for vec_Y_ (update) and vec_R_ (initial residual). Extra matvec is done to evaluate the final residual after adding update to initial guess.
  2. In the event initial residual > rhs, we default to refining Ax = b instead of computing an update due to limited precision and conditioning of the problem.

@ASunil-17 ASunil-17 requested a review from shakedregev October 24, 2025 14:26
@ASunil-17
Copy link
Collaborator Author

Have removed the comment from GramSchmidt and now I have included the changes to Rand FGMRES

rhsnorm = std::sqrt(rhsnorm);
if (rnorm > rhsnorm)
{
std::cout << "Initial guess is invalid. Refining Ax = b instead of Ay = r" << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the initial guess is bad, throw it out. Do not refine it.


if (rnorm > rhsnorm)
{
std::cout << "Initial guess is invalid. Refining Ax = b instead of Ay = r" << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Discard bad guess.

{
vec_z.setData(vec_Z_->getData(j, memspace_), memspace_);
vector_handler_->axpy(&h_rs_[j], &vec_z, x, memspace_);
vector_handler_->axpy(&h_rs_[j], &vec_z, vec_Y_, memspace_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

vec_y

Copy link
Collaborator

@shakedregev shakedregev left a comment

Choose a reason for hiding this comment

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

Old tests aren't passing, no new tests, incorrect functionality, minor formatting issues.

@ASunil-17
Copy link
Collaborator Author

Apologies for confusion. I was under the impression that successful compilation of the code meant that it passed some of the criteria on the template. I have changed it now.

Thank you for the feedback, I will remove the iterative refinement of Ax = b when initial residual > rhs. With regards to the tests, I recall you had advised me to run ctest -j before creating a PR. I apologise for not following that. I am in fact failing tests 30 and 35. It seems to be the same zero norm error for GramSchmidt, which I thought I had avoided.

Also I will create new tests for this feature. I am thinking of leveraging the sys_fgmres and syd_rand_fgmres tests to make new ones where I can test handling of successful breakdown, large initial residual, and so on. The changes I have made to the FGMRES and RandFGMRES seem to be passing the old tests, so I am optimistic I can make a successful PR.

@ASunil-17
Copy link
Collaborator Author

Hello Shaked,

I hope this email finds you well! I have fixed the zero norm handling in GramSchmidt and LinSolverIterativeFGMRES to gracefully exit and to compute the update in that situation. Keep in mind that this is happening after normalising the initial residual and iteratively refining based on that. These changes seem to be passing all the old tests successfully. I am currently figuring out how to create new tests. I know I want to test the early residual norm exit condition that stops us from using a bad initial guess. I am unsure how to integrate it within testSysGmres and testRandGmres

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.

3 participants