Skip to content
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

Bugfix: numerical_setup! for PETScLinearSolver #104

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JordiManyer
Copy link
Member

@JordiManyer JordiManyer commented Oct 1, 2024

We previously threw away PETSc's matrix ns.B, and re-set the KSP object (commented code).
This is not only unnecessary, but can also cause some issues with MUMPS.

I am not completely sure why, but here are some notes on the issue:

  • It is matrix-dependent, and only happens in parallel (nprocs > 1).
  • It has to do with the re-use of the symmetric permutation created by MUMPS to
    find the pivots.
    I think it probably re-orders the matrix internally, and does not re-order it again when we swap it
    using KSPSetOperators. So when accessing the new (non-permuted) matrix using the old permutation,
    it throws a stack overflow error.

In fact, when updating the SNES setups in the nonlinear solvers, we do not re-set the matrix,
but use copy! instead.

@amartinhuertas
Copy link
Member

We previously threw away PETSc's matrix ns.B, and re-set the KSP object (commented code).
This is not only unnecessary

I agree there is no need to do so (threw away ns.B), as the (implicit) assumption is that A has the same sparsity pattern as ns.B. I would not check that this is the case (it is costly), but let the code run even if not fulfilled. I guess that the copy! method will fail because of that?

@check_error_code PETSC.KSPSetOperators(ns.ksp[],ns.B.mat[],ns.B.mat[])
# ns.A = A
# ns.B = convert(PETScMatrix,A)
# @check_error_code PETSC.KSPSetOperators(ns.ksp[],ns.B.mat[],ns.B.mat[])
Copy link
Member

Choose a reason for hiding this comment

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

are we sure that we do no longer need to call KSPSetOperators?

Copy link
Member Author

@JordiManyer JordiManyer Oct 2, 2024

Choose a reason for hiding this comment

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

This is what the manual says:

To solve successive linear systems that have different preconditioner matrices 
(i.e., the matrix elements and/or the matrix data structure change), the user 
must call KSPSetOperators and KSPSolve for each solve.

which would indicate we do. However, my tests point to this not being the case. This works, for instance:

solver = PETScLinearSolver(mykspsetup)
println("First call")
ns = numerical_setup(symbolic_setup(solver,A),A)
x = allocate_in_domain(A)
fill!(x,0.0)
solve!(x,ns,b)

println("Second call")
map(Ai -> rmul!(Ai,2.0), partition(A))
b *= 2.0
numerical_setup!(ns,A)
x2 = allocate_in_domain(A)
fill!(x2,0.0)
solve!(x2,ns,b)

e = norm(x-x2)
println("Error =", e)

# ns.B = convert(PETScMatrix,A)
# @check_error_code PETSC.KSPSetOperators(ns.ksp[],ns.B.mat[],ns.B.mat[])
@assert nnz(ns.A) == nnz(A) # This is weak, but it might catch some errors
copy!(ns.B,A)
Copy link
Member

Choose a reason for hiding this comment

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

In this copy! call, which are the possible types for A?

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, all supported types

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.

2 participants