PreconditionRelaxation: automatically determine omega #16742
Conversation
There was a problem hiding this comment.
This PR introduces a relatively large amount of copy-paste. I would prefer if we could try to unify more code by base classes or free functions, to reduce the duplicated code? That said, it does not necessarily have to happen now in this PR, if it happens in the near future.
| @@ -2756,7 +2756,7 @@ AffineConstraints<number>::copy_from( | |||
| lines.clear(); | |||
| lines.reserve(other.lines.size()); | |||
|
|
|||
| for (const auto l : other.lines) | |||
| for (const auto &l : other.lines) | |||
There was a problem hiding this comment.
This is unrelated, or does it have to do with the other code in the PR, e.g. the test? (I don't think we need to be overly formal and open another PR, I'll approve it with this).
include/deal.II/lac/precondition.h
Outdated
|
|
||
| /** | ||
| * Constraints to be used for the operator given. This variable is used to | ||
| * zero out the correct entries when creating an initial guess. | ||
| */ | ||
| AffineConstraints<double> constraints; | ||
|
|
||
| /** | ||
| * Stores the preconditioner object that the Chebyshev is wrapped around. | ||
| */ | ||
| EigenvalueAlgorithm eigenvalue_algorithm; | ||
|
|
||
| /** | ||
| * Preconditioner. | ||
| */ | ||
| std::shared_ptr<PreconditionerType> preconditioner; |
There was a problem hiding this comment.
Given that the comments given here are the same as in the Chebyshev class (this looks like copy-paste), would it make sense to introduce a base class and let PreconditionChebyshev::AdditionalData inhert from that class? I fear that having the same members and same documentation two times will lead to divergence with hard-to-understand concepts in the future. Inheritance should also be completely backwards compatible.
There was a problem hiding this comment.
I have introduced a base class. PreconditionRelaxation::AdditionalData now adds relaxation and n_iterations and PreconditionChebyshev::AdditionalData degree and polynomial_type. What do you think?
| PreconditionRelaxation<MatrixType, PreconditionerType>::estimate_eigenvalues( | ||
| const VectorType &src) const | ||
| { | ||
| Assert(eigenvalues_are_initialized == false, ExcInternalError()); | ||
|
|
There was a problem hiding this comment.
Also here it appears as if we have redundancy and copy-paste from the other code. Could you try to unify more?
There was a problem hiding this comment.
The relevant lines of codes are already shared via the internal function internal::estimate_eigenvalues. PreconditionRelaxation and PreconditionChebyshev need to do some additional/different steps since 1) PreconditionRelaxation does not store the vectors and 2) the estimated eigenvalues are used differently. What would you move additionally to the internal function?
| protected: | ||
| /** | ||
| * Pointer to the matrix object. | ||
| */ | ||
| SmartPointer<const MatrixType, PreconditionRelaxation<MatrixType>> A; | ||
|
|
||
| /** | ||
| * Relaxation parameter. | ||
| * Stores the additional data passed to the initialize function, obtained |
There was a problem hiding this comment.
| * Stores the additional data passed to the initialize function, obtained | |
| * Store the additional data passed to the initialize function, obtained |
There was a problem hiding this comment.
I left this how it was since there multiple places in the documentation which use stores the same way.
|
@kronbichler Thank you for the comments to improve the docu. I have applied the changes. Let me know whether it is fine now. I will squash the comments after that. |
peterrum
force-pushed
the
relaxation_omega
branch
from
ad17fda
to
f4f94e2
Compare
Description of the problem The eigenvalue estimation for the smoother of the multigrid preconditioners was done through a PreconditionChebyshev. Description of the solution Due to recent changes in deal.II (dealii/dealii#16742), now we are able to do it using the PreconditionRelaxation directly. This allows us to remove the estimate omega function since it is done internally by the smoother if the relaxation parameter is set to 1. This allows also to remove the degree parameter that was only used in the case of Chebyshev.
by reusing the infrastructure used by
PreconditionChebyshev