Create SchurApproximation class so BlockSchurPreconditioner can be used for both AMG and GMG.#6806
Conversation
|
Sadly, this requires c++17: |
quangx
force-pushed
the
gmg_preconditioner_2
branch
2 times, most recently
from
cf29d33 to
ca6b8b9
Compare
| const SchurComplementMatrixType &Schur_complement_block, | ||
| const bool do_solve_Schur_complement, | ||
| const double Schur_complement_tolerance | ||
| ): |
gassmoeller
left a comment
gassmoeller
left a comment
There was a problem hiding this comment.
I like the intention, but I dont quite understand. If this PR unifies the use of BlockSchurPreconditioner and BlockSchurGMGPreconditioner, shouldnt the second be deleted? But I dont see this as a change in this PR. Or is it still used in the GMG-GC solver?
Some comments otherwise, but just about code style.
|
|
||
|
|
There was a problem hiding this comment.
This is not a strict rule, but we usually use 3 empty lines between function implementations, even if they are in header files. So please keep these empty lines.
| internal::SchurApproximation<GMGPreconditioner, StokesMatrixType, SchurComplementMatrixType, VectorType> schur_approximation_cheap(prec_Schur,stokes_matrix, | ||
| Schur_complement_block_matrix, /*do_solve_Schur*/ false, this->get_parameters().linear_solver_S_block_tolerance); | ||
|
|
||
| internal::SchurApproximation<GMGPreconditioner, StokesMatrixType, SchurComplementMatrixType, VectorType> schur_approximation_expensive(prec_Schur,stokes_matrix, | ||
| Schur_complement_block_matrix, /*do_solve_Schur*/ true, this->get_parameters().linear_solver_S_block_tolerance); |
There was a problem hiding this comment.
break these lines similar to the ones above to make them easier to read
I do intend to delete the BlockSchrGMGPreconditioner class. I am debugging this PR currently because tests are failing. I will address the other feedback you gave, thank you for looking at this! |
| // but the matrix-based version is a SparseMatrix::vmult() that requires passing the | ||
| // individual blocks. | ||
| if constexpr (std::is_same_v<VectorType, dealii::LinearAlgebra::distributed::BlockVector<double>>) | ||
| BT_operator.vmult(tmp, dst); |
There was a problem hiding this comment.
try to set tmp=0; before this vmult.
There was a problem hiding this comment.
If you need tmp=0 before the application of vmult, then you have a bug in the implementation of vmult: It represents an assignment to tmp, and whatever is in that vector before the call to vmult should not matter.
| } | ||
| else | ||
| { | ||
| schur_preconditioner.vmult(dst,src); |
|
@quangx Wolfgang is of course right: we should try to avoid zeroing out memory as much as possible for performance reasons. We now know that zeroing out data is enough to make all tests pass in this PR. Now you can try to remove as many of them as possible. |
quangx
force-pushed
the
gmg_preconditioner_2
branch
from
edccb4a to
cc117b3
Compare
quangx
force-pushed
the
gmg_preconditioner_2
branch
from
4847120 to
9340e45
Compare
|
can you apply this diff? https://ci.tjhei.info/job/aspect/job/PR-6806/35/artifact/changes-test-results.diff |
quangx
force-pushed
the
gmg_preconditioner_2
branch
from
79d8660 to
c4c20a8
Compare
|
@gassmoeller Could you take a look at this pull request again? You are correct, we need to keep the BlockSchurGMGPreconditioner class around for global coarsening (another pr). Thank you for your feedback! |
|
I think this is good to go now. Thank you, Quang. |
|
Looks good to me @quangx. |
4 participants
Currently ASPECT uses separate classes, BlockSchurPreconditioner and BlockSchurGMGPreconditioner, for AMG and GMG respectively. This pull request refactors the code so only BlockSchurPreconditioner is used in both cases.