Started the Convergence system #28848

joshuahansel · 2024-10-14T21:26:51Z

This PR revives #26744.

This creates a new system: Convergence. See #24128.

I'm putting this up now so that I can document some TODOs. Because it's in a messy state, I plan to squash everything into one commit before merging.

Tasks that remain:

Consider any necessary changes for the checkConvergence() API. Currently I have iteration index as the one and only argument, since we may want to use the same convergence class for multiple types of convergence check, and different types of convergence check will need to obtain that index differently. The same argument can be applied to others. Also I think that some classes will only be compatible with certain types of convergence check. We would probably want to do some checking to make sure it's used in a compatible way. Perhaps we should have some kind of IterationType argument? I'm open to other ideas.
UPDATE: Opened Make Convergence object checks aware of solve type #28927 for this task.
Understand the role of EquationSystems - does it do anything to set convergence parameters in there? Should Convergence being doing that? Some parameters in ResidualConvergence are unclear on if they're actually used, like the linear solve parameters. They get set in the EquationSystems object (which might be eventually set to PETSc), but at least some do not seem to come back into the class in the convergence check, maybe only getting used in libMesh's own convergence checks?
UPDATE 1: The EquationSystems object is populated with the convergence-related parameters and then used in various convergence checks of different solvers, which do not all have the freedom to specify a generic convergence criteria. My plan is to not interact with the EquationSystems object from the Convergence classes if it is not necessary, and let the Executioner parameters populate it, as they do currently.
UPDATE 2: The eigen executioners modify convergence-related parameters through EquationSystems, so it looks like we'll need to work with EquationSystems in the Convergence object after all.
Throw an error if the user supplies any shared convergence parameter to the executioner when they supply a ResidualConvergence? The current behavior is to allow the executioner parameters but override them. That's maybe the simplest implementation-wise, but seems very error-prone for users.
UPDATE: I plan to give only a warning if the user specifies nonlinear convergence parameters in the executioner when they have a non-default convergence.
Improve PostprocessorConvergence to perform checks of execute_on. Here the Convergence would need to know what kind of iterations it will be used with, so we need to think about how to do this.
Update: Opened Perform checks on execute_on for PostprocessorConvergence #28928 for this task.
Address any relevant comments from Convergence system #26744.

I opened another issue related to this: #28936.

moosebuild · 2024-10-15T00:44:35Z

Job Documentation, step Docs: sync website on 5a5abc3 wanted to post the following:

View the site here

This comment will be updated on new commits.

moosebuild · 2024-10-16T01:14:32Z

Job Coverage, step Generate coverage on 5a5abc3 wanted to post the following:

Framework coverage

	b8db75	#28848 5a5abc
	Total	Total	+/-	New
Rate	85.08%	85.09%	+0.01%	86.61%
Hits	106569	106759	+190	569
Misses	18694	18706	+12	88

Diff coverage report

Full coverage report

Modules coverage

Contact

	b8db75	#28848 5a5abc
	Total	Total	+/-	New
Rate	90.45%	90.41%	-0.05%	89.87%
Hits	4936	4957	+21	71
Misses	521	526	+5	8

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

framework new line coverage rate 86.61% is less than the suggested 90.0%
contact new line coverage rate 89.87% is less than the suggested 90.0%

This comment will be updated on new commits.

joshuahansel · 2024-10-23T22:46:34Z

I'm considering renaming ResidualConvergence to FEProblemDefaultConvergence since it does more than check residual norms. It has tolerances for xnorm and snorm too, for example.

joshuahansel · 2024-10-30T13:46:22Z

OpenMPI failure is a lie. Invalidate makes different tests fail.

joshuahansel · 2024-10-30T14:03:42Z

@GiudGiud @lindsayad Ready for review

GiudGiud

quick part 1 review

you ll want to mention Convergence in NonlinearSystem.md at
## Convergence Criteria for the Nonlinear System

lots of functions longer than 6 lines 🤔

framework/doc/content/source/actions/AddDefaultConvergenceAction.md

framework/doc/content/source/actions/AddConvergenceAction.md

framework/doc/content/source/convergence/FEProblemConvergence.md

framework/doc/content/source/convergence/IterationCountConvergence.md

framework/doc/content/syntax/Convergence/index.md

framework/include/actions/AddDefaultConvergenceAction.h

framework/include/convergence/IterationCountConvergence.h

joshuahansel · 2024-10-30T17:38:23Z

lots of functions longer than 6 lines

Most of those are inherited technical debt 😄 For example, there was a bunch of cut and paste from problems into convergences. My long-term plan here is to destroy these convergences in favor of smaller pieces. I see these convergences (e.g., FEProblemConvergence) as temporary objects for backwards-compatibility. Then, refactored, smaller convergences replace them in time.

GiudGiud · 2024-10-30T19:23:59Z

Yes that makes sense. You already cut out the "min/max" iter based convergence from that object.

GiudGiud

looks good.

framework/include/convergence/ReferenceResidualConvergence.h

framework/include/interfaces/ReferenceResidualInterface.h

GiudGiud · 2024-10-30T19:32:11Z

framework/src/base/Moose.C

@@ -200,6 +200,8 @@ addActionTypes(Syntax & syntax)
  registerMooseObjectTask("add_control",                  Control,                   false);
  registerMooseObjectTask("add_partitioner",              MoosePartitioner,          false);

+  registerMooseObjectTask("add_convergence",              Convergence,            false);


I would move this line up with other executioner-related objects, like time stepper and time integrator

GiudGiud · 2024-10-30T19:36:29Z

framework/src/convergence/PostprocessorConvergence.C

+  InputParameters params = IterationCountConvergence::validParams();
+
+  params.addClassDescription(
+      "Checks convergence based on absolute and relative error of the residual.");


well there s also the iteration count

is iteration count going to be available to every convergence object?
I would rather a different behavior, closer to the ComboTimestepper, where you specify multiple convergence object and there is a default "Combo" object that gets automatically added and makes sense

but the way you have it now has its merits too. One can turn off a criterion on a PP based on the number of iterations

Looks like that class description is garbage copy/paste.

I don't know about the combo idea since in this case it has parameters (e.g., max iterations).

Another design I considered was having IterationCountConvergence be a wrapper class that takes the name of another convergence, rather than having other convergences inherit from it. But that'd be pretty inconvenient for the user to make at least two convergences all of the time.

The IterationCountConvergence would not be the ComboConvergence

The comboConvergence would be created in the back end. See ComboTimeStepper for how it would work.

So you're suggesting that we don't make it an intermediate class but a final child class? Then somehow combine them? For the combo thing, I understand how that works, but here it's a little less clear because you don't necessarily know which Convergence objects will be for which iteration type (unless we have the user supply an iteration type).

we don't make it an intermediate class but a final child class

I would not make it final because I would want users to be able to derive from it. But yes, I dont see it be intermediary to every other class?

, but here it's a little less clear because you don't necessarily know which Convergence objects will be for which iteration type

you should know when you specify "nonlinear_convergence =" in the Executioner.

framework/doc/content/source/convergence/FEProblemConvergence.md

modules/contact/include/convergence/AugmentedLagrangianContactConvergence.h

modules/contact/src/problems/AugmentedLagrangianContactProblemInterface.C

modules/contact/src/problems/AugmentedLagrangianContactProblem.C

modules/contact/src/convergence/AugmentedLagrangianContactConvergence.C

lindsayad · 2024-10-31T17:48:29Z

framework/doc/content/source/convergence/IterationCountConvergence.md

@@ -0,0 +1,24 @@
+# IterationCountConvergence


Is this an object you or someone else actually wants?

Right now, it's kind of serving as a commonly used intermediate base class (PostprocessorConvergence inherits from it, for example). I think we want it in some form.

I d use it for debugging for sure. Setting a fixed number of iterations is helpful

lindsayad · 2024-10-31T17:51:13Z

framework/include/convergence/Convergence.h

+    CONVERGED = 2,
+    DIVERGED = -2


why 2 and not 1?

It was something Oana did, and I never changed. I'll change it

lindsayad · 2024-10-31T17:52:40Z

framework/include/convergence/Convergence.h

+   *
+   * @param[in] iter   Iteration index
+   */
+  virtual MooseConvergenceStatus checkConvergence(unsigned int iter) = 0;


Why are we treating the iteration counter specially? I thought we had discussed that this method should take no arguments? It seems like a convergence class that needs this information should be able to retrieve it for themselves

Yeah this relates to one of my discussions in the PR description. I'm trying to figure out what the best design will be when it comes time to extend the system to include other iteration types (like fixed point or steady-state detection). How might we use one generic Convergence for multiple iteration types?

It d be cool to be able to use the same CV classes for all the iterations

which likely precludes retrieving everything in the class

Yeah for generic CV classes, I definitely wouldn't want a different class for each type of iteration. I'd want to somehow distinguish within checkConvergence() the type of iteration, whether it gets passed in as an argument like IterationType, or whether each object is associated with a single iteration type. Maybe the user tells it, or maybe it is determined internally from nonlinear_convergence = my_conv.

I think I would feel better about having it as a parameter if you did not also have this IterationCountConvergence intermediate class. The latter's existence suggests to me that there is something insufficient about the base class despite the fact that there is an it parameter. If we are going to have this it parameter, I don't see why we need the intermediate class

Also, can we stop using CV to stand for convergence? Whenever I see it I just think const-volatile

Also, can we stop using CV to stand for convergence? Whenever I see it I just think const-volatile

man these requests 😆

you did not also have this IterationCountConvergence intermediate class

A good way to get rid of the intermediate class imo would be to use a "Combo" like behavior between multiple convergence classes, as is done for TimeSteppers. This would be automatically set up from all the Convergence objects specified to the "nonlinear_convergence" parameter of the FEProblem

I dont think I like that IterationCountConvergence is an intermediate class either. I would see it as a child class the same level as PPConvergence. But giving the PPConvergence one more criteria it can apply does not really hurt it either

I think "IterationCount" should be folded into the base class. It makes sense to me since the base class interface takes the it parameter

"Folded in" meaning that I think the functionality should be coded in the Convergence class, and that functionality can be called by child classes if they wish to incorporate iteration counting as part of their convergence criteria

So you mean all Convergence objects have the min/max parameters? And maybe raise the max default to the numeric max to completely disable by default? I think it could work. It just might not make sense for all of them to have the parameters, like the ParsedConvergence. There's always suppressParam I guess.

lindsayad · 2024-10-31T17:53:08Z

framework/include/convergence/FEProblemConvergence.h

+  /**
+   * Performs setup necessary for each call to checkConvergence
+   */
+  virtual void nonlinearConvergenceSetup(){};


Suggested change

virtual void nonlinearConvergenceSetup(){};

virtual void nonlinearConvergenceSetup() {}

lindsayad · 2024-10-31T17:53:45Z

framework/include/convergence/FEProblemConvergence.h

+
+protected:
+  FEProblemBase & _fe_problem;
+


Suggested change

lindsayad · 2024-10-31T17:57:08Z

framework/include/convergence/FEProblemConvergence.h

+  virtual bool checkRelativeConvergence(const PetscInt it,
+                                        const Real fnorm,
+                                        const Real ref_norm,
+                                        const Real rel_tol,
+                                        const Real abs_tol,


The interface should be consistent with respect to using Petsc types or "libMesh" types. I think unsigned int for it is good. You can use cast_int to make sure nothing bad happens

lindsayad · 2024-10-31T17:58:12Z

framework/include/convergence/IterationCountConvergence.h

+   *
+   * @param[in] iter   Iteration index
+   */
+  virtual MooseConvergenceStatus checkConvergenceInner(unsigned int iter);


Now here having this argument makes sense with a class name of IterationCountConvergence

Refs idaholab#24128

joshuahansel · 2024-10-31T20:21:46Z

@GiudGiud @lindsayad Ready for another round. I believe I've addressed your comments, so please check them off to confirm.

Refs idaholab#24128

joshuahansel mentioned this pull request Oct 14, 2024

Convergence system #26744

Closed

joshuahansel force-pushed the convergence-josh branch from fef1637 to 01cd337 Compare October 14, 2024 22:23

GiudGiud assigned joshuahansel Oct 15, 2024

joshuahansel force-pushed the convergence-josh branch from 01cd337 to 551e2ae Compare October 15, 2024 22:08

moosebuild added the PR: Failed but allowed label Oct 16, 2024

joshuahansel force-pushed the convergence-josh branch from 551e2ae to edb0c33 Compare October 24, 2024 18:55

moosebuild removed the PR: Failed but allowed label Oct 24, 2024

This was referenced Oct 24, 2024

Make Convergence object checks aware of solve type #28927

Open

Perform checks on execute_on for PostprocessorConvergence #28928

Open

Check that multiple conflicting Convergence objects do not exist #28936

Open

joshuahansel force-pushed the convergence-josh branch 4 times, most recently from c942aa4 to 2b00bb4 Compare October 29, 2024 12:08

joshuahansel marked this pull request as ready for review October 29, 2024 21:16

joshuahansel requested review from lindsayad, cticenhour, bwspenc, recuero, GiudGiud and roystgnr as code owners October 29, 2024 21:16

GiudGiud requested changes Oct 30, 2024

View reviewed changes

GiudGiud requested changes Oct 31, 2024

View reviewed changes

lindsayad requested changes Oct 31, 2024

View reviewed changes

oanaoana and others added 4 commits October 31, 2024 15:19

Started Convergence system

c64b1b2

Refs idaholab#24128

Renamed ResidualConvergence to FEProblemConvergence

3ba142c

Only allowed one FEProblemConvergence

9b25a57

Refs idaholab#24128

Error if parameter supplied in both convergence and executioner

115d6da

Refs idaholab#24128

joshuahansel force-pushed the convergence-josh branch from 542dd04 to 5a5abc3 Compare October 31, 2024 20:20

Addressed review

0920858

Refs idaholab#24128

joshuahansel force-pushed the convergence-josh branch from 5a5abc3 to 0920858 Compare November 5, 2024 13:48

	virtual void nonlinearConvergenceSetup(){};
	virtual void nonlinearConvergenceSetup() {}

Started the Convergence system #28848

Are you sure you want to change the base?

Started the Convergence system #28848

Conversation

joshuahansel commented Oct 14, 2024 • edited Loading

moosebuild commented Oct 15, 2024 • edited Loading

moosebuild commented Oct 16, 2024 • edited Loading

Framework coverage

Modules coverage

Contact

Full coverage reports

Warnings

joshuahansel commented Oct 23, 2024

joshuahansel commented Oct 30, 2024

joshuahansel commented Oct 30, 2024

GiudGiud left a comment

Choose a reason for hiding this comment

joshuahansel commented Oct 30, 2024

GiudGiud commented Oct 30, 2024

GiudGiud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiudGiud Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuahansel commented Oct 31, 2024

joshuahansel commented Oct 14, 2024 •

edited

Loading

moosebuild commented Oct 15, 2024 •

edited

Loading

moosebuild commented Oct 16, 2024 •

edited

Loading

GiudGiud Oct 31, 2024 •

edited

Loading