Split time-stepper-history cleaning from update #6019
Conversation
Integrating at the wrong order should only happen during self-start, and we don't do dense output during self-start.
Cleanup how happens at the end of the step, after the update and dense output, in both LTS and GTS modes. This allows the LTS time steppers to keep less data between steps.
| /// - Modifies: | ||
| /// - Tags::HistoryEvolvedVariables<variables_tag> | ||
| template <typename System> | ||
| struct CleanHistory { |
There was a problem hiding this comment.
Just a note while I'm thinking about it: we should revisit all of our actions and see how much we can convert into Mutators, maybe even ones that can take a db::Access to move a lot of code into cpp files. This one looks like a possible candidate, which is what made me think of it. I don't think we should change anything in this PR, just noting it so I/we don't forget :)
There was a problem hiding this comment.
We might need to template on dimension to get the mortar data tag (currently it searches DbTags), but otherwise I don't see any issues with making this a mutator.
There was a problem hiding this comment.
Sounds good! A future problem, but just wanted to note it :)
| @@ -68,6 +77,22 @@ struct CleanHistory { | |||
| }, | |||
| make_not_null(&box)); | |||
|
|
|||
| // This action can't depend on Evolution, but most of the time | |||
There was a problem hiding this comment.
Sorry, what below depends on evolution that the above doesn't?
There was a problem hiding this comment.
For forward-declared stuff, there's imex::protocols::ImexSystem and imex::Tags::ImplicitHistory (from Imex, not directly Evolution, I guess), and evolution::dg::Tags::MortarDataHistory (actually in the helper above). For completely not-named stuff, const auto history on line 122 has a type including evolution::dg::MortarData.
(Really, a lot (all?) of the stuff in the Time directory should probably be merged into Evolution, but that sounds painful.)
|
Hmm, do you understand the clang-tidy error? Tthis looks like it's telling us to change the STL? |
|
I think it's just complaining about charm stuff, but the charm class happens to be stored in a I haven't examined the charm code in detail, but it looks like it's doing reference counting, which can cause false positives for this sort of automated analysis, so it's probably fine. |
|
Sounds good! Merging now :) |
Reduces the amount of data we need to store in checkpoints, mostly in LTS.
Proposed changes
Upgrade instructions
Evolution executables now need to include the
Actions::CleanHistoryaction after each variable update. See the modifications to the executables in this PR for suggested exact changes.Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments