Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SNES convergence reason #29050

Open
joshuahansel opened this issue Nov 11, 2024 · 5 comments
Open

Fix SNES convergence reason #29050

joshuahansel opened this issue Nov 11, 2024 · 5 comments
Assignees
Labels
C: Framework T: task An enhancement to the software.

Comments

@joshuahansel
Copy link
Contributor

joshuahansel commented Nov 11, 2024

Motivation

Since #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 #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:

  • Do nothing
  • Give warning if user specifies -snes_converged_reason (and tell them about verbose)
  • Actually disable -snes_converged_reason

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.

@joshuahansel joshuahansel added C: Framework T: task An enhancement to the software. labels Nov 11, 2024
@joshuahansel joshuahansel self-assigned this Nov 11, 2024
@lindsayad
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

I think that's the best solution.

@joshuahansel joshuahansel changed the title Remove SNES convergence reason Fix SNES convergence reason Nov 12, 2024
@lindsayad
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Framework T: task An enhancement to the software.
Projects
Development

No branches or pull requests

2 participants