Add method to execute selected postprocessors - #7226
Conversation
bangerth
left a comment
bangerth
left a comment
There was a problem hiding this comment.
I'm not against this PR, but can you explain where this is useful? Running postprocessors is a tricky business because it also affects the statistics table, and running only some means filling a row in the table only partially. I'm pretty sure you have a good reason, but I'd like to talk about the purpose of this function before we merge :-)
| auto plugin = this->plugin_objects.begin(); | ||
| for (unsigned int i = 0; | ||
| i < this->plugin_names.size(); | ||
| ++i, ++plugin) | ||
| { | ||
| if (postprocessors_to_execute.find(this->plugin_names[i]) | ||
| == postprocessors_to_execute.end()) | ||
| continue; | ||
|
|
||
| for (const std::string &dependency : (*plugin)->required_other_postprocessors()) | ||
| if (postprocessors_to_execute.insert(dependency).second) | ||
| dependency_list_changed = true; |
There was a problem hiding this comment.
I must admit that I don't understand this loop. plugin_names are the names of the existing plugins. postprocessors_to_execute is initialized with the subset of postprocessors the caller wants called. So your loop looks like this:
for (plugin : list of all plugins)
{
if (plugin is not in postprocessors_to_execute)
continue;
// so were are now only looking at postprocessors that are in the caller-provided list
add all dependencies of plugin to the list
}
I think it would have been easier to actually turn this loop and the one below around: Go through all of the requested plugin names, find the corresponding plugin (or error out if it doesn't exist), and then ask that plugin for its dependencies which you add to the end of the list.
I don't feel adamant about this. If you like your version better, keep it but at least add some commentary that helps understand what the pieces of the loop are doing along the lines of my pseudocode above.
|
I will let @hangqjan explain the purpose of the function, but the issue about the statistics file is solved because he keeps a separate statistics table in the adjoint manager, that is specifically used for all postprocessor evaluations during adjoint computations. |
gassmoeller
left a comment
gassmoeller
left a comment
There was a problem hiding this comment.
Just a few more comments. I agree with Wolfgang that the loops in the function are very hard to understand. It would be great if they could be simplified or at least more extensively documented.
| // first call the update() function. | ||
| (*plugin)->update(); |
There was a problem hiding this comment.
Depending on how the discussion in #7235 is going you may be able to remove this. Leave it in for now, it doesnt hurt.
| dependency_list_changed = false; | ||
|
|
||
| auto plugin = this->plugin_objects.begin(); | ||
| for (unsigned int i = 0; |
There was a problem hiding this comment.
Please add a comment at the beginning of this loop describing what it does. At the moment I am unsure it does exactly what is intended.
|
And we will probably need a test for this pull request. Can you start from the test |
708723a to
1996b1c
Compare
3 participants
This PR adds a new method to
Postprocess::Managerfor executing a selected set of postprocessors.The change is extracted from #7120 to keep that PR focused and to make this functionality available independently. It is needed by the adjoint framework, where specific postprocessors must be executed on demand to provide the required forward-model quantities.
Before your first pull request:
For all pull requests:
I have reviewed, understood, and verified the codes generated by AI.
If yes, please describe your usage of AI models in the creation of this pull request
For new features/models or changes of existing features: