Add support for merging well cells for NLDD partitioning #6103
Conversation
|
jenkins build this opm-common=4537 please |
1 similar comment
|
jenkins build this opm-common=4537 please |
| // Fix-up for an extreme case: Interior cells whose neighbours are all | ||
| // on a different rank--i.e., interior cells which do not have on-rank | ||
| // neighbours. Make each such cell be a separate domain on this rank. | ||
| // Don't remove this code unless you've taken remedial action elsewhere | ||
| // to ensure the situation doesn't happen. For what it's worth, we've | ||
| // seen this case occur in practice when testing on field cases. | ||
| // The non-reachable cells are marked with -1 in the partition vector. | ||
| // Here we collect them into a single domain, per rank. |
There was a problem hiding this comment.
This comment appears to be a little inconsistent now. The first part says that we make each isolated cell be a separate single domain, while the last part says that we put all of these cells into one combined domain. Perhaps you can clarify the comment a little?
There was a problem hiding this comment.
It was misleading, indeed. It should be clearer now.
Note that before, we did not skip this domain. I mostly added this functionality as I experienced getting around 7 times as many domains as expected on a field case where there were many single isolated cells, which will be safe to skip. From the earlier comment, it sounded like the isolated cells could have neighbours on different ranks. But it should be safe to skip anyways, as NLDD would not update any solutions outside of this rank anyways. Right?
| const auto nDigit = 1 + static_cast<int>(std::floor(std::log10(comm.size()))); | ||
| auto partition_fname = odir / fmt::format("{1:0>{0}}", nDigit, comm.rank()); | ||
| std::ofstream pfile { partition_fname }; | ||
| if (!pfile) { |
There was a problem hiding this comment.
You're not supposed to need this check. The std::ofstream constructor already throws an exception if the file cannot be opened. I suppose there's something to be said for making this code robust in parallel where we might risk that some processes/ranks are able to open the file while others aren't. In that case we'd get weird hangs. Handling that, however, will require introducing parallel exception handling.
There was a problem hiding this comment.
Aha, that makes sense. Removed it now. I think there is work to be done for better parallel error handling and parallel output for NLDD, but I will not target that in this PR.
|
|
||
| // Breadth-first search for neighbor cells up to num_levels away | ||
| for (int level = 0; level < num_levels && !frontier.empty(); ++level) { | ||
| new_frontier.reserve(frontier.size() * 2); |
There was a problem hiding this comment.
There's a subtle problem here. Since you move() the new_frontier object into frontier below, it will be in a "valid but unspecified state" if this loop runs (at least) twice. I think I might personally write this loop using vector<>::swap() instead, i.e., something like
for (level) {
new_frontier.clear();
new_frontier.reserve(...);
for (cell) {
// ...
}
frontier.swap(new_frontier);
}which avoids that semantic pitfall.
There was a problem hiding this comment.
Good point. Adopted swap instead now.
jakobtorben
force-pushed
the
add_support_for_merging_well_cells_for_nldd_partitioning
branch
2 times, most recently
from
4fc91ea to
0988b37
Compare
|
jenkins build this please |
jakobtorben
force-pushed
the
add_support_for_merging_well_cells_for_nldd_partitioning
branch
from
0988b37 to
36cda5a
Compare
|
jenkins build this please |
| @@ -222,6 +241,9 @@ class BlackoilModelNldd | |||
| for (const int domain_index : domain_order) { | |||
| const auto& domain = domains_[domain_index]; | |||
| SimulatorReportSingle local_report; | |||
| if (std::find(domains_to_skip_.begin(), domains_to_skip_.end(), domain.index) != domains_to_skip_.end()) { | |||
There was a problem hiding this comment.
It is not a priori clear to me that it is a good idea to skip these domains of isolated cells, as a local solve might reduce newton iterations. However, given that this code has been tested thoroughly, I assume the skipping approach has been validated?
Could store a flag in the domain itself rather than doing a lookup here, to save a little time.
|
Did not notice it was merged while I looked at it... Anyway not problematic, but I would like the comments to be addressed in a small follow-up PR. |
NLDD Improvements for Partitioning and Better Handling of Wells and Isolated Cells
This PR introduces several improvements to the Nonlinear Domain Decomposition (NLDD) implementation, with a main focus on better handling cells perforated by wells during subdomain partitioning.
Improvements
Well Cell Merging
Well Neighborhood Control
local_domains_partition_well_neighbor_levels_which specifies the number of levels of neighbours to merge into one entity:Enhanced Partition Visualization
Code Consistency
local_domain_*tolocal_domains_*)Testing