PIDTransientControl update to better support recover #31845
Conversation
…re consistently on restore idaholab#31842
|
Job Documentation, step Docs: sync website on 0d21637 wanted to post the following: View the site here This comment will be updated on new commits. |
|
Job Coverage, step Generate coverage on 0d21637 wanted to post the following: Framework coverage
Modules coverageStochastic tools
Full coverage reportsReports
This comment will be updated on new commits. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
jthano
force-pushed
the
pid_transient_add_initial_setup
branch
from
dfca76e to
0d21637
Compare
|
Job Test, step Results summary on 0d21637 wanted to post the following: Framework test summaryCompared against 0cc44f8 in job civet.inl.gov/job/3354977. Removed testsAdded testsRun time changesModules test summaryCompared against 0cc44f8 in job civet.inl.gov/job/3354977. Removed testsAdded testsRun time changes |
| 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' |
There was a problem hiding this comment.
was that necessary? the regold is due to that right?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| void | ||
| PIDTransientControl::initialSetup() | ||
| { | ||
| if (_app.isRecovering()) |
There was a problem hiding this comment.
it's unusual to execute on initialSetup ?
why is this control the only one that needs to do that?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
even INITIAL is odd actually. You run without recover, the parameter is changed, you ran with recover, the parameter is not recovered
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
3 participants
closes #31842