Skip to content

PIDTransientControl update to better support recover #31845

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
Open

PIDTransientControl update to better support recover #31845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af1f2c4
PIDTransientControl update so it will initialize controlled values mo…
jthano Nov 4, 2025
0d21637
small test update that catches issue with restore and executing on ti…
jthano Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions framework/include/controls/PIDTransientControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ class PIDTransientControl : public Control

virtual void execute() override;

/**
* Called once at the beginning of the simulation, used to initialize
* recovered control values
*/
virtual void initialSetup() override;

private:
/// The current value of the target postprocessor
const PostprocessorValue & _current;
Expand Down
2 changes: 1 addition & 1 deletion framework/include/executioners/TransientBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ class TransientBase : public Executioner

/**
* Is the current step at a sync point (sync times, time interval, target time, etc)?
* @return Bool indicataing whether we are at a sync point
* @return Bool indicating whether we are at a sync point
*/
bool atSyncPoint() { return _at_sync_point; }

Expand Down
14 changes: 13 additions & 1 deletion framework/src/controls/PIDTransientControl.C
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ PIDTransientControl::execute()
// Compute the value, within the bounds
_value = std::min(std::max(_minimum_output_value, _value), _maximum_output_value);

// Set the new value of the postprocessor
// Set the new value of the parameter or postprocessor
if (isParamValid("parameter"))
setControllableValue<Real>("parameter", _value);
else
Expand All @@ -194,3 +194,15 @@ PIDTransientControl::execute()
_old_delta = delta;
}
}

void
PIDTransientControl::initialSetup()
{
if (_app.isRecovering())
Copy link
Contributor

@GiudGiud GiudGiud Nov 4, 2025

Choose a reason for hiding this comment

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

it's unusual to execute on initialSetup ?
why is this control the only one that needs to do that?

Copy link
Contributor Author

@jthano jthano Nov 4, 2025

Choose a reason for hiding this comment

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

There appear to not be many controls that work on parameters and the point of this is obviously only to restore parameter values after recovery. I think the only other control that works on parameters is RealFunctionControl. And perhaps initialSetup should be added to RealFunctionControl because if it is executing on timestep_end, it will have the same problem that PIDTransientControl has, restore won't work properly because the controlled parameter will be reset to it's initial value instead of the last value it had before recovery.

Copy link
Contributor Author

@jthano jthano Nov 4, 2025

Choose a reason for hiding this comment

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

Normally, restoring something that was computed on timestep_end is not a problem because the data item would be declared recoverable. But the problem in general with controllable things is that the control object has the recoverable data, not the controlled object. So controlled objects like parameters do not recover like recoverable data. I think using initialSetup is okay, but maybe there is a better way to enforce proper recovery. The problem seems to be that a developer may add another control object that works on parameters, but then not add an initialSetup method to do a proper restore. I think we need a common base class for controls that work on parameters and that base class will have an initialSetup method that is marked final that does restore properly. But since there are so few controls that work on parameters, I don't know if that should be done now

Copy link
Contributor

@GiudGiud GiudGiud Nov 5, 2025

Choose a reason for hiding this comment

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

I think we need a common base class for controls that work on parameters

we would need to track the names and types of the parameters in that base class to have the ability to set them on initialSetup. Types might be fine, there are only so many types that are controllable.

timestep_end is also not the only problem flag, it's anything that is not executed before the first recovered step occurs.

Copy link
Contributor

@GiudGiud GiudGiud Nov 5, 2025

Choose a reason for hiding this comment

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

even INITIAL is odd actually. You run without recover, the parameter is changed, you ran with recover, the parameter is not recovered

Copy link
Contributor Author

@jthano jthano Nov 5, 2025

Choose a reason for hiding this comment

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

even INITIAL is odd actually. You run without recover, the parameter is changed, you ran with recover, the parameter is not recovered

Yes executing on INITIAL along with TIMESTEP_END would not properly recover as the code is because there is an early return if the app is recovering and the early return happens before the control actually sets the controlled objects.

timestep_end is also not the only problem flag, it's anything that is not executed before the first recovered step occurs.

This is a reason I'd like to implement something similar to my solution, that is using initialSetup() which one would expect to always be called regardless of the execution flags

Copy link
Contributor

@GiudGiud GiudGiud Nov 6, 2025

Choose a reason for hiding this comment

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

Yes executing on INITIAL along with TIMESTEP_END

even without timestep_end.

This is a reason I'd like to implement something similar to my solution, that is using initialSetup() which one would expect to always be called regardless of the execution flags

I don't think we can impose in C++ that initialSetup is ALWAYS called by the derived class, unless we override final it and offer an initialSetupDerived() virtual routine for the derived classes

{
if (isParamValid("parameter"))
setControllableValue<Real>("parameter", _value);
else
_fe_problem.setPostprocessorValueByName(getParam<std::string>("parameter_pp"), _value);
}
}
8 changes: 6 additions & 2 deletions framework/src/problems/FEProblemBase.C
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1095,7 +1095,7 @@ FEProblemBase::initialSetup()
for (THREAD_ID tid = 0; tid < n_threads; ++tid)
checkUserObjectJacobianRequirement(tid);

// Check whether nonlocal couling is required or not
// Check whether nonlocal coupling is required or not
checkNonlocalCoupling();
if (_requires_nonlocal_coupling)
setVariableAllDoFMap(_uo_jacobian_moose_vars[0]);
Expand Down Expand Up @@ -1487,6 +1487,7 @@ FEProblemBase::initialSetup()
}

// Control Logic
_control_warehouse.initialSetup();
executeControls(EXEC_INITIAL);

// Scalar variables need to reinited for the initial conditions to be available for output
Expand Down Expand Up @@ -5265,7 +5266,10 @@ FEProblemBase::executeControls(const ExecFlagType & exec_type)

if (!ordered_controls.empty())
{
_control_warehouse.setup(exec_type);
// already called by initialSetup when exec_type == EXEC_INITIAL
if (exec_type != EXEC_INITIAL)
_control_warehouse.setup(exec_type);

// Run the controls in the proper order
for (const auto & control : ordered_controls)
control->execute();
Expand Down
8 changes: 4 additions & 4 deletions test/tests/controls/pid_control/gold/min_max_limits.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
time,integral,left_boundary_average
0,0,0
1,2.5,4
2,1.500000003131,2
3,1.5,2
4,1.5,2
1,0.5,0
2,2.075,3.15
3,2.5,4
4,1.84625,2.6925
5,1.5,2
6,1.5,2
7,1.5,2
Expand Down
5 changes: 3 additions & 2 deletions test/tests/controls/pid_control/tests
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[Tests]
[pid]
requirement = "The Control system shall be able to control an input parameter to make a postprocessor match a target value,"
issues = '#17271'
issues = '#17271 #31842'
design = 'syntax/Controls/index.md source/controls/PIDTransientControl.md'
[basic]
type = 'CSVDiff'
Expand All @@ -16,7 +16,8 @@
type = 'CSVDiff'
input = 'pid_control.i'
csvdiff = 'min_max_limits.csv'
cli_args = 'Outputs/file_base=min_max_limits Controls/integral_value/maximum_output_value=4.0 Controls/integral_value/minimum_output_value=2.0 Executioner/nl_abs_tol=1e-14 Executioner/end_time=10'
cli_args = 'Outputs/file_base=min_max_limits Controls/integral_value/maximum_output_value=4.0 Controls/integral_value/minimum_output_value=2.0
Executioner/nl_abs_tol=1e-14 Executioner/end_time=10 Controls/integral_value/execute_on=timestep_end'
Copy link
Contributor

@GiudGiud GiudGiud Nov 4, 2025

Choose a reason for hiding this comment

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

was that necessary? the regold is due to that right?

Copy link
Contributor Author

@jthano jthano Nov 4, 2025

Choose a reason for hiding this comment

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

I can undo this change and add a new test instead. Changing this test so PIDTransientControl executes on timestep end allows this existing test to catch the issue noted in #31842

Copy link
Contributor

@GiudGiud GiudGiud Nov 5, 2025

Choose a reason for hiding this comment

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

yes let s have another test.
Feel free to just duplicate this one
otherwise it's too hard to verify that the test covers a bunch of independent requirements the next time we modify it

detail = 'allowing the Proportional Integral Derivative control to have limits for its maximum and minimum output,'
[]
[abs_max_rate_change]
Expand Down