Make sure that Structure::MultigridPreconditioner is updated

[richardschu]

fixes #652 The important bit is in https://github.com/richardschu/exadg/blob/04428487cb27f3faf8ad74e64fdd99cb8af3a92d/include/exadg/structure/preconditioners/multigrid_preconditioner.cpp#L125 where we now have:

  // In case that the operators have been updated, we also need to update the smoothers and the
  // coarse grid solver. This is generic functionality implemented in the base class.
  if(nonlinear or data.unsteady or this->update_needed)
  {
    this->update_smoothers();
    this->update_coarse_solver();
  }

The this->update_needed() was missing before, but the block behind the if is to be executed as well.

and also adds some tests to check some of the most important preconditioners. The application was modified to get cheaper tests.

[nfehn]

Thanks for figuring this out. In my opinion, there are way too many new test cases in this PR. Shouldn't there actually be one test case enough for this tiny change in the code?

[richardschu]

Thanks for figuring this out. In my opinion, there are way too many new test cases in this PR. Shouldn't there actually be one test case enough for this tiny change in the code?

I would vote for testing what we do not test elsewhere, I can reduce the number of tests therefore, but I think quite some combinations are never tested.

[kronbichler]

In my opinion, there are way too many new test cases in this PR.

I think that we should considerably increase the number of tests, given that many combinations and steps are not tested. While it is hard to have definite numbers, I would consider 500-2000 tests for a project of this size not unreasonable. Of course, it would be necessary to structure our tests in a way to ensure rapid development cycles, e.g. by having a subset of quick tests or similar actions we can elaborate on. Having uncovered features is much more concerning in the longer run, which is why I am very much in favor of adding all tests that address previously untested features (of course avoiding redundancy within the suggested tests).

[nfehn]

In my opinion its too complex to test all combinations of parameter in ExaDG. Also the code does not have the complexity that all parameters are coupled and need to be tested in all the combinations. You found that the linear steady case is not tested, right? So why not just test this one?

Also related to your comment, @kronbichler: What I do not understand here is why we discuss the "big philosophical problem of testing" and not the problem we currently have here. It should also be part of the discussion that many many tests take part of our (expensive) development time and require resources for maintenance. Also right now as a reviewer, I actually need to invest too much time into such a tiny change. I am not complaining here, it's just that I believe there are more arguments to the discussion.

[nfehn]

As an addition: It might turn out that the if-statement can be removed entirely. Shouldn't we then actually rethink the addition of many new tests here? So as a suggestion for the future: Creating a pull request with changing just the line of the if-statement, briefly discussing this (and whether we need tests for this or not; as I said, I am not sure if we already have the final variant regarding the if-statement; it might be that the code can be "simplified" compared to the previous version), and then continue (based on this discussion) could be more effective overall.

[richardschu]

As an addition: It might turn out that the if-statement can be removed entirely. Shouldn't we then actually rethink the addition of many new tests here? So as a suggestion for the future: Creating a pull request with changing just the line of the if-statement, briefly discussing this (and whether we need tests for this or not; as I said, I am not sure if we already have the final variant regarding the if-statement; it might be that the code can be "simplified" compared to the previous version), and then continue (based on this discussion) could be more effective overall.

Yes, this is what I will check: https://github.com/exadg/exadg/pull/654#discussion_r1538940917 The cases tested elsewhere, I will also remove! Then we can have a look at whats left :)

[richardschu]

The if mentioned in https://github.com/exadg/exadg/pull/654#discussion_r1538940917 is actually never false, meaning we can simply omit the condition entirely (I checked that with all the tests I had previously implemented).

I removed the testcases using the JacobiPreconditioner, not because it is tested elsewhere, but to reduce the number of tests in our own interest. The combination finite strain + unsteady + multigrid prec is tested in the structure/manufactured applciation's tests, the ExcMessages I fixed are un-fixed now (separate PR).

Now do we want to further remove some tests? I do not see any redundancies. And yes, we can never get a coverage of 100%, but I think it might be useful, since one can always overlook some edge-cases. We can move some of the unsteady tests to the structure/manufacured to have a proper reference solution as well (here, we have a large error, since I cannot make the test cheap == very small number of time steps with the solution in this application).

[nfehn]

For me, the reasoning is not clear why to have "redundant" tests e.g. for AMG and "multigrid". Note that "Multigrid" still has many parameters and that "Multigrid" internally contains AMG, see e.g.:

  Multigrid type:                            phMG
  p-sequence:                                Bisect
  Smoother:                                  Chebyshev
  Preconditioner smoother:                   PointJacobi
  Iterations smoother:                       5
  Smoothing range:                           2.0000e+01
  Iterations eigenvalue estimation:          20
  Coarse grid solver:                        CG
  Coarse grid preconditioner:                AMG

How would you motivate the need for this from the code? If I understand the implementation correctly, the Structure module uses a "generic" AMG preconditioner, not a "structure-specific" AMG preconditioner, or do I misunderstand something? Assuming this is correct, the structure-specific parameters "steady/quasistatic/unsteady" and "small/large strain" are not really seen by the implementation of the AMG preconditioner used when running Structure simulations. Then, many tests could be considered superfluous. Generally, I consider it a con of the procedure suggested here that this gives somewhat exponential complexity in the number of tests w.r.t. parameters, and I am not sure if this really helps (given that the main purpose of most of the tests here seems to be "no segmentation fault occurred" while most of the parameter combinations are actually still untested (and where I argue that we cannot avoid this) or while the implementation seems to be somewhat insensitive to the variation of parameters being tested).

In my opinion, added tests will not be removed again in the future (because one might have to expect to do harm if removing them, arguing that the person who introduced them might have had "good reasons to do this"). We are now evaluating "good reasons to do this", hence I would like to ask you for your understanding that I would like to understand the reasoning for adding certain tests.

[richardschu]

@nfehn sorry I forgot about this, was focusing on teaching, conference, etc. ... we talked about this some time ago; as far as I remember, the conclusion was to remove redundant tests. In the current case: remove all the AMG tests, since the coarse grid solver of the multigrid is actually AMG. For the remaining combinations, I have to check was is actually tested in the other applications.

[richardschu]

OK update: tests in structure module are: manufactured: Unsteady, large strain weak_damping: Unsteady, large strain Therefore I am also pro removing the Unsteady tests added here.

[richardschu]

Let me know if you agree on this and then I will udpate. :smile_cat:

[nfehn]

Yes, I voted for removing tests as much as possible. (I would also be willing to merge the changes in the code (removal of one if-statement) without modifications in applications/tests, just to have an improved version of the code (without the identified bug) asap.)

[richardschu]

I kept the small strain unsteady example, since this is not tested elsewhere. The remaining tests are: manufactured: unsteady, large strain weak_damping: unsteady, large strain + weak damping terms bar:

1. quasistatic solver + large strain + Multigrid/AMG coarse grid preconditioner
2. steady solver + small AND large strain + Multigrid/AMG coarse grid preconditioner
3. unsteady solver + small strain + Multigrid/AMG coarse grid preconditioner

Then we have all the solver options tested with no extra shenanigans! :1st_place_medal: