Skip to content

LinearSolver: update after ConvergenceError #4144

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pbrubeck
Copy link
Contributor

@pbrubeck pbrubeck commented Mar 21, 2025

Description

Fixes #4143

@pbrubeck pbrubeck requested a review from jrmaddison March 21, 2025 21:50
@pbrubeck pbrubeck force-pushed the pbrubeck/fix/linear-solver branch from f17f70d to b337d9d Compare March 21, 2025 22:11
except firedrake.exceptions.ConvergenceError:
assert solver.ksp.getConvergedReason() == PETSc.KSP.ConvergedReason.DIVERGED_MAX_IT

assert not numpy.allclose(u.dat.data_ro, uinit.dat.data_ro)
Copy link
Contributor

Choose a reason for hiding this comment

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

u.dat.data_ro is zero here, i.e. the zero initial guess, rather than the result from one CG iteration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial guess is -1.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's zero, as ksp_initial_guess_nonzero defaults to False.

@pbrubeck
Copy link
Contributor Author

LinearSolver used to just wrap a KSP, but it now became a NonlinearVariationalSolver, so we need to decide whether we want to preserve the old behavior or to accomodate to the usual way of providing initial guesses to NLVS and always use whatever is stored in the solution.

ksp_initial_guess_nonzero applies only to the Newton update step, that stores the update as an internal PETSc.Vec, which is reused across different solve() calls, so the default ksp_initial_guess_nonzero=False makes sense here.

@jrmaddison
Copy link
Contributor

I didn't realize that was the behaviour. Does that mean a LinearVariationalSolver always uses a non-zero initial guess for the (single step) Newton solve, and then zero guesses the increment solve? i.e. ksp_initial_guess_nonzero has no effect for a LinearVariationalSolver?

@pbrubeck pbrubeck changed the title LinearSolver: fix zero initial guess and update after error LinearSolver: update after ConvergenceError Mar 26, 2025
@pbrubeck
Copy link
Contributor Author

pbrubeck commented Mar 26, 2025

I didn't realize that was the behaviour. Does that mean a LinearVariationalSolver always uses a non-zero initial guess for the (single step) Newton solve, and then zero guesses the increment solve?

This is correct.

i.e. ksp_initial_guess_nonzero has no effect for a LinearVariationalSolver?

The effect is very subtle, and is only for the update solve, ksp_initial_guess_nonzero=True will use the previous update vector as the initial guess for the current update vector.

@pbrubeck
Copy link
Contributor Author

Upon discussing this during the meeting, LinearSolver will use as initial guess whatever is passed to solve(x, b) to be consistent with NLVS and LVS. So this PR will only deal with alwats updating the solution, even when an Exception is raised.

@jrmaddison
Copy link
Contributor

Upon discussing this during the meeting, LinearSolver will use as initial guess whatever is passed to solve(x, b) to be consistent with NLVS and LVS. So this PR will only deal with alwats updating the solution, even when an Exception is raised.

Sounds good, thanks.

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.

BUG: LinearSolver convergence failures discard results
2 participants