Skip to content

Conversation

@joshuahansel
Copy link
Contributor

  • Added class ComponentsConvergence that checks convergence of all components
  • Added new method Component::getNonlinearConvergence() to return that Component's convergence, if any
  • Have not actually overridden the above method for any Component yet, just introducing concept for review

Refs #31038

@joshuahansel joshuahansel self-assigned this Jul 15, 2025
@joshuahansel
Copy link
Contributor Author

@lindsayad @GiudGiud @licharlot For anyone interested, I'm putting up this PR for the concept to be reviewed. This is how I'm planning to handle convergence for Components.

I'm still working on the pieces I plan to use for the individual convergence checks. Here's generally what I'm thinking might be "good" criteria:

  1. Max iterate change of each variable (say, {p, T, vel}) in that Component's block(s) less than some tolerance (provided by parameters like max_change_T)
  2. Require a loose absolute residual check to mitigate false convergence due to possible convergence stalling (in which case condition 1 would report convergence). Do our best to normalize the residual to a standard error. For example, do some kind of nondimensionalization, and correct any domain size or mesh size biases. For example, for drhoA/dt, normalize by rho_ref A (and then mesh size bias).

@GiudGiud
Copy link
Contributor

i like the idea. I think this mirrors the component-scaling that was done in SAM but without modifying the nonlinear solve performance

@moosebuild
Copy link
Contributor

moosebuild commented Jul 15, 2025

Job Documentation, step Docs: sync website on 75bfb3d wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Jul 15, 2025

Job Coverage, step Generate coverage on 75bfb3d wanted to post the following:

Framework coverage

bb0a08 #31039 75bfb3
Total Total +/- New
Rate 86.02% 86.02% -0.00% 100.00%
Hits 124559 124560 +1 5
Misses 20244 20245 +1 0

Diff coverage report

Full coverage report

Modules coverage

Thermal hydraulics

bb0a08 #31039 75bfb3
Total Total +/- New
Rate 88.58% 88.73% +0.15% 100.00%
Hits 14730 15025 +295 308
Misses 1899 1908 +9 0

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensical to me

@joshuahansel joshuahansel force-pushed the components-convergence branch from 21be217 to f64f739 Compare August 28, 2025 20:08
@joshuahansel joshuahansel force-pushed the components-convergence branch 5 times, most recently from a74a5c2 to 7945b8a Compare October 10, 2025 16:43
Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a formal review, just an early curious look with some suggestions. I can see how great this is going to be now! This should bring the long-sought robustness to THM. or at least help

}

bool
FEProblemBase::hasMaterial(const std::string & name, const THREAD_ID tid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FEProblemBase::hasMaterial(const std::string & name, const THREAD_ID tid)
FEProblemBase::hasActiveMaterial(const std::string & name, const THREAD_ID tid)

considering the implementation below


This [IterationCountConvergence.md] can be used with the [Components system](Components/index.md) to check the convergence of all Components that return a Convergence object with `getNonlinearConvergence()`. The inner convergence check returns its convergence status as follows:

- `CONVERGED`: All Components returning a Convergence object are `CONVERGED`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `CONVERGED`: All Components returning a Convergence object are `CONVERGED`
- `CONVERGED`: All Convergence objects created by Components are `CONVERGED`

same for the other?

@@ -0,0 +1,14 @@
# (AD)FlowModel1PhaseFunctorMaterial

This [functor material](/FunctorMaterials/index.md) computes the following quantities
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This [functor material](/FunctorMaterials/index.md) computes the following quantities
This [functor material](/FunctorMaterials/index.md) creates functor material properties for the following quantities

can you add something like "the names of the functor material properties" are specified by ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you need to use a listing of THM.h to get the names

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add the names of the properties manually

Comment on lines 3 to 4
This post-processor normalizes [ElementExtremeFunctorValue.md] by a provided
constant, for use by [FlowChannel1PhaseConvergence.md].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of that PP does not really convey this function. Could be renamed imo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I struggled with making it a reasonable length, but I'm open to suggestions. I decided to name it based on concept

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just add a scaling factor for ElementExtremeFunctorValue ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that

const std::vector<std::pair<std::string, Real>> var_norm_pairs{
{THM::PRESSURE, _p_ref}, {THM::TEMPERATURE, _T_ref}, {THM::VELOCITY, _vel_ref}};
for (const auto & var_norm_pair : var_norm_pairs)
addTHMNormalizedVariableStep(var_norm_pair.first, var_norm_pair.second);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not seem 1 phase or THM specific? Maybe it should be in a ConvergneceUtils

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The step PP itself you mean? Not sure what you're suggesting with ConvergenceUtils. I had considered modifying ElementExtremeFunctorValue but didn't want to "pollute" it. I also considered just using an existing PP to scale, but didn't want the 2nd PP existing (it'd probably be ok though).

return Convergence::MooseConvergenceStatus::DIVERGED;
else if (status == Convergence::MooseConvergenceStatus::ITERATING)
all_converged = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe check else == converged

so we are future proof if a 4th option is created for MooseConvergenceStatus::

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an assert should be enough to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel pretty good about keeping these 3 options into eternity - to me it feels like a basic design of the system

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even basic design can change

eternity sounds like a very long time. and who knows where AI-driven completely automated development will take moose..

Comment on lines 36 to 37
addPressureFunctorProperty<true>();
addPressureFunctorProperty<false>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need both? what for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to take better notes... I'll put some comments in the code after I refresh my memory. But basically error messages would come up regarding mismatch of AD and non-AD.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok let s document who s requesting this stuff in both flavors. that might be a hidden issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like only the non-AD is needed, so I'll just add that for now. I'm guessing that earlier, I was trying to leverage the functor property in some other places.

Comment on lines 43 to 45
template <bool is_ad>
void
FlowModel1PhaseFunctorMaterial::addPressureFunctorProperty()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all these templates should move to the source, as you are not using them from another file


const auto variable = getParam<VariableName>("variable");
if (variable == "rhoA")
return rho_ref * A_ref * h_min;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason for h_min rather than L_component?
is the residual proportional to 1/dx for THM solves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's targeted at the residual for each nonlinear equation, which is proportional to dx (finite volume).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the residual for each cell. how do we get the residual for the whole equation? Is an operation more like infinite norm or sum of absolute residuals?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base class computes a discrete norm (l1, l2, or linf) of the element residuals for that variable. I choose linf.

@joshuahansel joshuahansel force-pushed the components-convergence branch from 7945b8a to 75bfb3d Compare November 3, 2025 18:57
@moosebuild
Copy link
Contributor

Job Test, step Results summary on 75bfb3d wanted to post the following:

Framework test summary

Compared against bb0a08b in job civet.inl.gov/job/3353620.

Removed tests

Added tests

Run time changes

Modules test summary

Compared against bb0a08b in job civet.inl.gov/job/3353620.

Removed tests

Added tests

Test Time (s)
thermal_hydraulics/test:components/flow_channel_1phase.err:components_convergence_scaling 1.62
thermal_hydraulics/test:components/flow_channel_1phase.components_convergence 1.04

Run time changes

Test Base (s) Head (s) +/-
thermal_hydraulics/test:problems/brayton_cycle.open 4.23 6.89 +62.77%
thermal_hydraulics/test:components/inlet_velocity_t_1phase.clg:velocity_t_3eqn 3.16 4.95 +56.79%
thermal_hydraulics/test:problems/area_constriction.without_junction 3.18 4.92 +54.89%
stochastic_tools/test:web_server_control.stochastic_control/normal 3.30 5.03 +52.64%
thermal_hydraulics/test:components/inlet_stagnation_p_t_1phase.phy:p0T0_3eqn 2.35 3.57 +52.08%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants