refactor the network iteration code #6095
Conversation
|
jenkins build this please |
|
jenkins build this failure_report please |
totto82
force-pushed
the
refactor_cleanup
branch
from
bedbcba to
8c38541
Compare
|
jenkins build this failure_report please |
1 similar comment
|
jenkins build this failure_report please |
|
Thanks for the request. I will try to begin reviewing it tomorrow. |
|
There are some warnings need to be fixed, but it can be done after the reviewing is done. |
| const auto& summaryState = simulator_.vanguard().summaryState(); | ||
| std::vector<Scalar> pot(this->numPhases(), 0.0); | ||
| const Group& fieldGroup = this->schedule().getGroup("FIELD", reportStepIdx); |
There was a problem hiding this comment.
I do not think the renaming the reportStepIdx and local_deferredLogger should be part of this PR.
And I think when possible, using reportStepIdx might be less confusing. But I acknowledge there is many usage of episodeIdx in this file.
| bool updateWellControlsAndNetworkIteration(const bool mandatory_network_balance, | ||
| const int network_update_iteration, | ||
| const double dt, | ||
| bool& well_group_control_changed, |
There was a problem hiding this comment.
Although it is not a big thing, I do not see we need to change the interface of this function. The original form is more compact and without introducing new modified argument.
| const int episodeIdx = simulator_.episodeIndex(); | ||
| const auto& comm = simulator_.vanguard().grid().comm(); | ||
| this->updateAndCommunicateGroupData(episodeIdx, iterationIdx, param_.nupcol_group_rate_tolerance_, deferred_logger); | ||
| bool more_inner_network_update = updateNetwork(mandatory_network_balance, network_update_iteration, deferred_logger); |
| const int iterationIdx = simulator_.model().newtonMethod().numIterations(); | ||
| const int episodeIdx = simulator_.episodeIndex(); | ||
| const auto& comm = simulator_.vanguard().grid().comm(); | ||
| this->updateAndCommunicateGroupData(episodeIdx, iterationIdx, param_.nupcol_group_rate_tolerance_, deferred_logger); |
There was a problem hiding this comment.
In the original code, this communication is done after
if (!this->wellsActive() && !network.active()) {
return {false, false};
}Do you think it is potentially introducing some extra communications?
| const std::string network_str = more_inner_network_update? "": "Network did not converge."; | ||
| const std::string gaslift_str = alq_updated? "": "Gaslift optimization oscillates."; | ||
| const std::string wg_str = well_group_control_changed? "": "Group/well controls oscillates."; | ||
| bool output_to_info = !this->shouldBalanceNetwork(episodeIdx, iterationIdx + 1) && more_inner_network_update; |
| return {more_network_update, well_group_control_changed}; | ||
| // but after certain number of the iterations, we terminate | ||
| const std::size_t max_iteration = param_.network_max_outer_iterations_; | ||
| if (network_update_iteration + 1 == max_iteration) { |
There was a problem hiding this comment.
I like the original way that this type of process is in the general iteration workflow. Basically we reach the maximum allowed number of iterations, then we do some process. It is part of the processing of the iteration process.
The code in the current way is putting this process inside the iteration function itself, it is harder to follow.
If you want to keep the function updateWellControlsAndNetwork tider, you can wrap this process in a separate function.
There was a problem hiding this comment.
The motivation for this refactoring is to allow for more fine-grained checks for convergence of the outer network iterations as well as more informative output. See PR #6100 which part of the same issue is addressed. That is also why I would like to move the iteration workflow inside the function.
There was a problem hiding this comment.
Thanks for the response. I see you want to output the information regarding more_inner_network_update and alq_updated. That should be okay if you want to get them from the function updateWellControlsAndNetworkIteration() for outputting purpose.
In my idea is that
while (do_network_update) {
if (network_update_iteration > maximum_iteration) {
output message
break;
}
do_network_update = updateWellControlsAndNetworkIteration();
++network_update_iteration;
}In its current form from this PR, inside the function updateWellControlsAndNetworkIteration(network_update_iteration), we update things (do the iteration first), then oh, ++network_update_iteration > maximum_iteration, we should not do the future iterations anymore, let us output some messages, then return false. I do think deciding no more iteration within the **Iteration() function itself is less clear, and the **Iteration() function get unnecessary length and complication.
totto82
force-pushed
the
refactor_cleanup
branch
from
8c38541 to
1a4ba55
Compare
|
jenkins build this please |
| fmt::format("The maximum of {} outer network iterations is reached. Stop iteration. Caused by:" | ||
| "{}" | ||
| "{}" | ||
| "{}" |
There was a problem hiding this comment.
I think you can use concatenations here, Caused by:{}{}{}{}"
| // only output to terminal if we are at the last newton iterations where we try to balance the network. | ||
| // the network is not yet balanced | ||
| const std::string network_str = more_inner_network_update? "": "Network did not converge."; | ||
| const std::string gaslift_str = alq_updated? "": "Gaslift optimization oscillates."; |
There was a problem hiding this comment.
For those strings, You might want a space at the beginning, so the causes are not output without a space between them.
|
I will close this now and instead focus on getting #6100 in. |
This decouples the group/well constraint check and the network update. The motivation was to get better output to user and .DBG file.