Clean up implementation in SolverBicgstab #14257
Conversation
| class SolverBicgstab : public SolverBase<VectorType>, | ||
| protected internal::SolverBicgstabData | ||
| class SolverBicgstab : public SolverBase<VectorType> |
There was a problem hiding this comment.
This is possibly an incompatible change, but since the base class was marked as internal, I don't think we need to create a way out for the user.
kronbichler
force-pushed
the
cleanup_bicgstab
branch
from
e9b1484
to
f99a9f8
Compare
|
If we would go into the operations on vectors, similarly to #13698, we could get out quite some additional performance, also without introspection into the preconditioner. (I will not do that here, because I have not critical use case of it for now, but I wanted to push some changes I had accumulated recently.) |
| @@ -408,13 +341,13 @@ SolverBicgstab<VectorType>::iterate(const MatrixType & A, | |||
|
|
|||
| preconditioner.vmult(y, p); | |||
| A.vmult(v, y); | |||
| rhobar = rbar * v; | |||
| if (std::fabs(rhobar) < additional_data.breakdown) | |||
| const double rbar_dot_v = rbar * v; | |||
There was a problem hiding this comment.
double is fine? Shouldn't it be VectorType::value_type?
There was a problem hiding this comment.
I agree, but then I think we should do it for all occurrences (omega, alpha, etc). This class won't currently work for complex numbers without proper restructuring (I am not even sure if BiCGStab does work theoretically), so I am not sure I will be able to correctly resolve the ambiguity between VectorType::value_type and the associated real_type. I will do my best.
| IterationResult state(false, SolverControl::failure, 0, 0); | ||
| do | ||
| { | ||
| state = iterate(A, preconditioner); | ||
| state = iterate(A, x, b, preconditioner, state.last_step); |
There was a problem hiding this comment.
Personally, I would suggest to inline this function (in a follow-up PR).
There was a problem hiding this comment.
I actually tried to move everything into solve, but then I was discouraged by the various places where the iterate function might jump out and request a new attempt with a clean residual and no history, so I gave up. I am sure it can be further restructured, but I realized it would take more time than I wanted to spend (which is probably better spent once we do the bigger task of integrating similar tricks as in the SolverCG and need to restructure the code a bit more).
There was a problem hiding this comment.
I see. Let's keep it like this for now.
kronbichler
force-pushed
the
cleanup_bicgstab
branch
from
4b124bf
to
aa2ebef
Compare
kronbichler
force-pushed
the
cleanup_bicgstab
branch
from
aa2ebef
to
c99d421
Compare
This commit removes one redundant inner product from
SolverBicgstab(t * tnear the end of the algorithm) and otherwise restructures the code: I do not like the style of having local class variables holding pointers to the vectors (despite the work I did in #14251 where I did actually some variables to the class), because theVectorMemoryclass should take care of it. Furthermore, there were some scalars as class variables, which makes it much more cumbersome to read the code than if we define them at the place of use. All of these changes should also be seen against the background that the typical use case ofBiCGStabis that we enter theiteratefunction only once; the outer iteration is, as far as I can see, only there to catch some rather artificial breakdown that is not seen on large systems. As a result, the code to hold things in the class variables seems like premature optimization to me.I had to add some arguments to the
iteratefunction in this process, but I think this makes the code clearer, so even that seems like a clear win.