Add and test skip_times option for Predictor #6507
Conversation
|
Since this is a new feature in the framework - we really should have a test in the framework for it. Can you come up with a simple one? Maybe have a jump in a BC on a transient diffusion calculation or something? |
|
@friedmud I'll do that. I think the best test for this is to check what the residual is right after the predictor is applied (rather than counting the nonlinear iterations, which the current test does). I'll see if I can cook up a postprocessor that lives in the MOOSE tests that does that. |
|
I just pushed a new version of the commit to add a set of framework tests for the predictor. To test this more reliably, I added options to the Residual postprocessor to enable it to output the initial residual (either before or after applying preset BCs). That code is all tested by the new framework tests that I added for the predictor. |
| @@ -73,6 +73,10 @@ AdamsPredictor::apply(NumericVector<Number> & sln) | |||
| // that gets called on time step begin. | |||
| // That means that history control must go here. | |||
| historyControl(); | |||
|
|
|||
| if (!shouldApply()) | |||
There was a problem hiding this comment.
I would rather have this handled by the framework calling shouldApply() and then just not calling Predictor::apply() if it's false...
There was a problem hiding this comment.
That historyControl() wouldn't happen if it were done that way. Based on the comment above that call, it sounds like we'd need to call a method on the predictor at the beginning of the time step to do what you suggest (which probably isn't terribly difficult). Is that what we should do?
There was a problem hiding this comment.
Maybe split this up... give it an initialize(), execute() (or apply()) and finalize() just like other objects in MOOSE?
initialize() could be called all the time... then shouldApply() will be checked... the last two only get called if shouldApply() returns true.
Just a suggestion. If no one else thinks it matters then it's probably fine how it is ;-)
There was a problem hiding this comment.
That looks really easy to do. We could add a timestepSetup() method to the predictor that gets called from NonlinearSystem::onTimestepBegin(), and move everything in AdamsPredictor::historyControl() into that method. Once we did that, we could have the framework call shouldApply() and only call apply() if that's true. I'll go ahead and do that unless I hear otherwise.
There was a problem hiding this comment.
It looks like our wires got crossed. So is the name initialize() preferred to timestepSetup()? Would it get called at the beginning of the timestep or when the predictor is about to be applied? Either way works.
There was a problem hiding this comment.
I like timestepSetup() better for now... let's go that direction.
We can add an initialize() later if its needed. timestepSetup() is even more standard across MOOSE.
|
The version I just pushed has the API changes for Predictor that I discussed with @friedmud in this thread. |
Modify existing test in solid_mechanics and add version of the test to skip the predictor on a step. Add similar framework tests. Also add and test options to specify the residual type in the Residual postprocessor to allow us to more reliably test the predictor.
|
This is fine with me. |
Add and test skip_times option for Predictor
closes #6506