Add vector copy to ilu preconditioner.

[fritzgoebel]

This vector copy ensures linearity of the preconditioner when iterative solvers are used for the triangular systems.

Without the additional copy, the solution vector is used as initial guess to the second solver, no matter its content. When only doing a few iterations, this initial guess still has a strong influence on the result and, depending what is in the respective initial guess vecotrs, we cannot assume the following equality:
prec(x_1) + prec(x_2) = prec(x_1 + x_2)

(here, prec(x) means applying the preconditioner to a vector x)

Especially for preconditioned GMRES, this leads to errors in the result.

[upsj]

LGTM, great catch!

[pratikvn]

LGTM! This would affect only when you use an iterative method as a tri-solver right ? Is the overhead of copying acceptable in the case we use a normal direct cusparse tri-solver ?

[hartwiganzt]

@pratikvn Yes, I would argue that the overhead is acceptale.

[thoasm]

LGTM

[yhmtsai]

I think we copy b to cache (set_cache_to(b)) to make sure the cache is not random filled before.
Should the cache get data from x not b here?
first apply will use x as initial guess
second apply will use cache as initial guess

[fritzgoebel]

I'm not sure that I completely understand the question, but I'm fairly certain that the data from x should not be used since it can be any vector, e.g. the krylo vector from a previous iteration. This doesn't matter for exact triangular solves since they overwrite x with the solution anyway. However, when using iterative triangular solves, this causes problems. The first apply solves:
Ly = b
By setting the cache to b, we use the right hand side as initial guess. Now the second apply solves
Ux = y
and by copying x from the cache, we again use the right hand side as initial guess.

[pratikvn]

But I agree with @yhmtsai that x might be a better choice because, in the case of the solving Ly=b you are kind of hard-coding the init guess to b. But if you set the cache to x, would it not be possible for the function calling the preconditioner apply to set the initial guess and if you need it then you can set it to b, right ?

[fritzgoebel]

I see what you mean. However, using the data from x as initial guess for the first solve introduces the same nonlinearity as before, only in a different spot. This way, we would have to ensure linearity of the preconditioner, a "linear operator", from outside the preconditioner before calling it, by putting adequate restrictions to x.
I checked and tried GMRES with the following configurations (without writing extra ode in GMRESpreparing the data in x):

• only set_cache_to(b): this was the initial version, and gives errors with iterative triangular solves
• only set_cache_to(x): also gives errors in the result
• set_cache_to(x) and x->copy_from(cache_.intermediate.get()): also gives errors in the result
• set_cache_to(b) and x->copy_from(cache_.intermediate.get()): This is the only configuration for which the result GMRES computes is correct when using a few block Jacobi sweeps as triangular solves in the preconditioner.

[fritzgoebel]

I typed up an explanation, hopefully this clarifies:

[yhmtsai]

a question about the initial guess of first apply

[upsj]

I just thought about it some more, and am now only more confused about this issue.
According to the LinOp interface, apply(b, x), respectively x=op(b), the linear solver should compute x only based on b.
So with my interpretation of the interface, an iterative solver should only use values from b, but not x.
This also means that the set_cache_to(b) seems superfluous (except for setting the size of the cache) and the iterative solver should use its own internal cache(s) initialized with the right-hand side.
Can you tell me where I am going wrong here?

[codecov]

Codecov Report

Merging #506 into develop will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #506      +/-   ##
===========================================
+ Coverage    88.52%   88.62%   +0.09%     
===========================================
  Files          268      268              
  Lines        16907    16907              
===========================================
+ Hits         14967    14983      +16     
+ Misses        1940     1924      -16     

Impacted Files	Coverage Δ
include/ginkgo/core/preconditioner/ilu.hpp	65.78% <100.00%> (+0.92%)	⬆️
reference/components/format_conversion.hpp	100.00% <0.00%> (ø)
include/ginkgo/core/base/name_demangling.hpp	88.88% <0.00%> (+3.17%)	⬆️
include/ginkgo/core/matrix/hybrid.hpp	84.21% <0.00%> (+10.37%)	⬆️
omp/components/format_conversion.hpp	100.00% <0.00%> (+60.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be2eb46...cdbbab4. Read the comment docs.

[hartwiganzt]

We are applying a preconditioner to the Krylov vector s to get the preconditioned Krylov search direction p. So, in fact, we want to solve Ap=s - or more precisely LUp=s.

Now, applying a direct solver, we would just solve Lt=s (forward) and Up=t (backward) with t being some temporary vector.

If we want to iteratively, we use some iteration method L_method->apply(s,t), but here the initial guess plays a role. A smart initial guess for t is the right-hand side s.
The same for the upper solve: U_method->apply(t,p) and t is a good initial guess for p.

[upsj]

I withdraw my concerns, but we should definitely mention that LinOp::apply(b, x) might still use the values of x.

[hartwiganzt]

I withdraw my concerns, but we should definitely mention that LinOp::apply(b, x) might still use the values of x.

Please add a comment where you feel it is appropriate!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information