Some comments on the type constraint of PreconditionRelaxation::step

[RichardYCJ]

Hi @peterrum ,

Got a few comments regarding to your commit about preconditioner relaxation.

First of all, great appreciation for your job to add type constraints to step method for different preconditioner.

I notice that you use the template overload to check whether MatrixType has a XXX_step method and skip the relaxation step if not implemented.

template <typename VectorType,
          typename std::enable_if<!has_jacobi_step<MatrixType, VectorType>,
                                  MatrixType>::type * = nullptr>
void step(VectorType &, const VectorType &) const {
  Assert(false,
         ExcMessage("Matrix A does not provide a Jacobi_step() function!"));
}

From my point of view, it is better to expose the error on compile time rather than on runtime, since there's nothing we can do in this situation. (maybe check whether precondition_XXX exists?)

Besides, the exception would only be thrown under Debug config by using Assert, which would make it hard to diagnose.

For example, PreconditionJacobi::step will not break up but gives out different results when initialized by SparseMatrix and MatrixFreeOperator::Base (no Jacobi_step but only precondition_Jacobi is implemented).

Additionally, we may use this type constraints to do some compile-time selection. For instance, to make a MGSmootherAuto to automatically choose MGSmootherRelaxation if XXX_step is provided and fall back to MGSmootherPrecondition if only precondition_XXX is available, as long as we expose this constraint to upper interfaces.

Of course, using concept constraints offerred by c++20 will be a more elegant way but it still take time to bring this feature down to real production coding.

[peterrum]

First of all, great appreciation for your job to add type constraints to step method for different preconditioner.

:) The goal of the PRs was to improve PreconditionRelaxation so that one can switch more easily preconditioners, just like in the Chebyshev case.

Besides, the exception would only be thrown under Debug config by using Assert, which would make it hard to diagnose.

In #13958, I switched from Assert to AsserThrow. I played around with static asserts but they gave me some false positives. Does this solution work for you?

[RichardYCJ]

The goal of the PRs ... one can switch more easily preconditioners ... I switched from Assert to AsserThrow.

I understand that if it is needed to supress the compile error while switching between preconditioners.

Though I still think to add type constraints and no overload to step is more expressive since we can do nothing about the preconditioner if MatrixType does not offer the step implementation. But this still needs thorough discussion in the community.

Using AssertThrow is more easily for developer to locate the problem. And I totally agree the current work around is the best solution at the moment. Thanks anyway.