idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.78k stars 1.05k forks source link

Fix SNES convergence reason #29050

Open joshuahansel opened 1 week ago

joshuahansel commented 1 week ago

Motivation

Since https://github.com/idaholab/moose/pull/28848, the behavior for the SNES converged reason is

  // convert convergence status to PETSc converged reason
  switch (status)
  {
    case Convergence::MooseConvergenceStatus::ITERATING:
      *reason = SNES_CONVERGED_ITERATING;
      break;

    case Convergence::MooseConvergenceStatus::CONVERGED:
      *reason = SNES_CONVERGED_FNORM_ABS;
      break;

    case Convergence::MooseConvergenceStatus::DIVERGED:
      *reason = SNES_DIVERGED_DTOL;
      break;
  }

Thus PETSc will sometimes report the wrong reason. However, note that PETSc sometimes bypasses some of this and assigns its own reason. For example, if you have a NaN or exceed the number of nonlinear iterations, you'll still get the correct, respective divergence reasons. But for example if you converge due to relative tolerance, it'll report it as SNES_CONVERGED_FNORM_ABS now.

This is being replaced with verbose output via https://github.com/idaholab/moose/pull/29049. However, we must decide what to do (if anything) with the SNES converged reason.

It seems the default behavior is to print the divergence reason only, and if the user specifies -snes_converged_reason additionally specify convergence reason as well.

Design

Some options:

Of course, if there are situations where Convergence objects are not replacing PETSc checks, then this should be omitted from the above.

Impact

Warnings likely.

lindsayad commented 1 week ago

I do not think we should disable the ability for a user to pass -snes_converged_reason. If that means we need to add something like SNES_CONVERGED_USER and SNES_DIVERGED_USER to PETSc, then I can do that

joshuahansel commented 1 week ago

Yeah I don't think a universal disable would be appropriate. I'm just thinking if we're able to detect that PETSc will return incorrect reason, we should at least warn.

What's the difference with SNES_CONVERGED_USER?

lindsayad commented 1 week ago

What do you mean? I would add this to the SNESConvergedReason enum: https://petsc.org/release/manualpages/SNES/SNESConvergedReason and we would associate 1 with SNES_CONVERGED_USER, 0 with SNES_CONVERGED_ITERATING, and -1 with SNES_DIVERGED_USER

joshuahansel commented 1 week ago

I think that's the best solution.

lindsayad commented 1 week ago

I've opened https://gitlab.com/petsc/petsc/-/merge_requests/8005