LGR: rebuild Cartesian->compressed map after refinement, enable serial summary output, fix rank-asymmetric throw - #7245
Conversation
hnil
added
manual:irrelevant
akva2
left a comment
akva2
left a comment
There was a problem hiding this comment.
Looks fine to me. Small suggestions.
|
Context for reviewers: part of a group of independent LGR fixes. Nothing here needs another PR to build or merge, but two are closely related:
|
There was a problem hiding this comment.
I have not looked at this in detail but I think it is not fine.
You have surely found bugs, but the way you are fixing them is not good and breaks the current architecture.
Updating cartesianToCompressed might hide more problems than it solves. It should simply not be used in certain situation directly.
I would recommend to post bug reports that clearly describe the problem.
| // unrefined grid) is now stale. Rebuild it before well connections | ||
| // are resolved, otherwise coarse-grid wells map to the wrong leaf | ||
| // cell (e.g. their source term lands on an unrelated cell). | ||
| this->updateCartesianToCompressedMapping_(); |
There was a problem hiding this comment.
The cartesianToCompressed mapping is still problematic with refinement as it is not 1:1 anymore for cells coming from refinement.
Maybe we should change that and only add entries for unrefined cells. Then it would be a mapping for
existing cells on level 0 only.
There was a problem hiding this comment.
This is what the branch now does - the rework landed the day after your review. cartesianToCompressed_ is built from level-zero cells only, so it is a mapping for unrefined cells exactly as you suggested, and it is rebuilt after refinement rather than kept from the pre-refinement load balance. The commit is "Cartesian->compressed map: level-zero cells only, rebuilt after refinement".
| const auto& mappers = ParentType::lgrMappers_.value(); | ||
| if (lgr_level < 0 || static_cast<std::size_t>(lgr_level) >= mappers.size()) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
If this happens then this is a programming bug and should be fixed elsewhere.
Like this we are just hiding it and making everything more dangerous...
There was a problem hiding this comment.
Agreed, and changed: compressedIndexForInteriorLGR now throws on a structural error instead of returning a sentinel. -1 is returned only for a cell that is genuinely absent on this rank, which is a legitimate answer rather than a hidden bug.
| const auto& mapper = mappers[lgr_level]; | ||
| const auto it = mapper.find(lgr_cartesian_index); | ||
| if (it == mapper.end()) { | ||
| return -1; // cell of this LGR is not present on this rank | ||
| } |
There was a problem hiding this comment.
Same change as the thread above - it is fatal on structural errors now.
ac2b54e to
add3473
Compare
|
Points taken on all four - the review was right on each. Reworked and force-pushed:
Underlying bugs written up in #7260 and #7261. Verified: serial Also folded in the two copy-paste suggestions: the grid-derived update sequence is one helper now, and the output condition in FlowProblemBlackoil is a single predicate. |
add3473 to
76eb86a
Compare
76eb86a to
c3bd6f1
Compare
|
What do I do with this @blattms. |
c3bd6f1 to
9c386d3
Compare
|
Measured what this unblocks, on
On master every cell-based output is zero once an LGR is present - not just FIP, also block quantities. This PR turns it back on; #7244 then closes the remaining ~1.4% (the refined cells' share), so refinement reproduces the unrefined field totals exactly. |
evalSummaryState (which runs both calc_inplace and summary.eval) and writeInitialFIPReport were skipped for any CpGrid with maxLevel()>0. As a result LGR (CARFIN) decks produced all-zero summary/PRT/XWEL output, and a RPTSCHED FIP report segfaulted because the inplace_ accumulator was never populated (Inplace::get(FIELD) on an empty map). In serial the CollectDataOnIORank index maps are the identity and the leaf-grid well/cell data is already written correctly, so the only thing missing was the summary evaluation itself. The unsupported case is exactly a *distributed* refined CpGrid, for which CollectDataOnIORank does not build its index maps. Put that condition in one private helper, eclOutputEvalSupported_(), and use it at both call sites, so future call sites cannot miss a condition (review suggestion). Output-only change: solver behaviour is unchanged (SPE1CASE1_CARFIN1 stays at 52 Newton iterations), and non-LGR runs are unaffected.
…r absent cells The lookup used std::map::at() twice, so any miss threw std::out_of_range. On a distributed grid the misses have very different meanings, and an exception here fires on some ranks and not others, which is a collective hazard during well setup. The rank-without-the-box case is reachable by construction: a COMPDATL box occupies part of the grid, and a rank holding no cell of it (interior or overlap) still resolves every LGR well when building the well containers. (Observed in development as a np>=6 deadlock on a faulted-box variant of SIMPLE_2PH_W_FAULT_LGR; that particular deck needs refinement across a fault, which CpGrid does not support yet, so it is not reproducible on master -- the reachable path is, via any COMPDATL deck whose box misses a rank.) Separate the miss cases per review: - LGR name unknown: every rank registers every requested LGR (with an empty level grid where it holds no box cells), so the level structure is identical on all ranks and a failed name lookup is a programming error. Fatal, with a message saying so. - Connection (i,j,k) outside the LGR's dimensions: validated at parse time, so reaching here with an out-of-range position is likewise a bug. Fatal. - Cell absent from this rank's level mapper: the one legitimate miss -- the box lives on other ranks and this rank's level grid is empty or holds a different part of it. Return -1, mirroring compressedIndexForInterior's contract for coarse cells; the global existence of every connection cell is checked collectively afterwards (checkAllConnectionsFound). Verified on SPE1CASE1_CARFIN_GR (COMPDATL well): serial and np=2 resolve the well through this path and run.
…ement Two related problems with cartesianToCompressed_ on a locally refined grid: 1. It was built once, during load balancing on the unrefined grid, and never again. Refinement renumbers the leaf, so afterwards every stored compressed index is stale and coarse-grid well connections resolve to the wrong leaf cell. On SPE1CASE1_CARFIN1 the PROD source term landed on cart 91 instead of the perforation cell cart 299 -- mass-conserving, so masked in serial, but a ~27x first-iteration CNV spike in parallel once reordering moved the misrouted residual onto a small refined cell. 2. Rebuilding naively is not enough, because on a refined grid a level-zero Cartesian index is no longer a unique key: every child of a refined cell reports its ancestor's index, so the map entry for a refined-away cell is whichever child was inserted last -- an arbitrary answer served silently. Per review, restrict the map to unrefined cells: it becomes a mapping for existing cells on level zero only. A level-zero index that has been refined away now resolves to "not present" (and downstream to the explicit cells-not-found handling) instead of to an arbitrary child; cells inside a refinement are addressed through the LGR-aware lookup. The map is cleared on rebuild since it can now legitimately be built twice. The post-grid-change updates (grid view, this map, cell depths, thicknesses) are factored into updateDerivedGridState_(), called from both loadBalance() and addLgrs(), so a future grid-changing step cannot miss one of them (review suggestion). Verified: PROD resolves to active index 923; first-iteration CNV matches the non-LGR baseline, identical serial vs parallel; serial/parallel summaries agree to 1e-6; unrefined decks are byte-identical (hasFather() is false everywhere, so the map is unchanged).
9c386d3 to
08afaff
Compare
|
jenkins build this please |
There was a problem hiding this comment.
Pull request overview
This PR fixes several CpGrid+LGR edge cases by (1) re-enabling serial ECL summary/FIP evaluation for refined grids, (2) making LGR well-connection lookup rank-safe (fatal vs -1 cases), and (3) rebuilding/clarifying Cartesian→compressed index mapping semantics after grid refinement/load balancing.
Changes:
- Centralizes the “is ECL summary/FIP evaluation supported?” decision in
FlowProblemBlackoiland uses it at both summary and initial-FIP call sites. - Reworks
compressedIndexForInteriorLGR()to avoid rank-asymmetric exceptions and to distinguish programming errors (fatal) from legitimate per-rank misses (-1). - Rebuilds derived grid state after refinement and restricts the Cartesian→compressed map to level-0 (unrefined) cells, clearing stale entries on rebuild.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
opm/simulators/flow/FlowProblemBlackoil.hpp |
Adds eclOutputEvalSupported_() helper and uses it to gate summary/FIP evaluation for distributed refined CpGrid. |
opm/simulators/flow/FlowBaseVanguard.hpp |
Clears and rebuilds Cartesian→compressed mapping after grid changes; filters mapping to unrefined (level-0) leaf cells. |
opm/simulators/flow/CpGridVanguard.hpp |
Makes LGR interior index lookup deterministic across ranks (fatal vs -1), adds derived-grid-state update helper, and rebuilds derived state after refinement. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!element.hasFather()) | ||
| { | ||
| unsigned cartesianCellIdx = cartesianIndex(elemIdx); | ||
| cartesianToCompressed_[cartesianCellIdx] = elemIdx; | ||
| } |
Reworked per review — thanks, the comments improved all three fixes. Same three bugs, different shapes:
1. Serial LGR summary/FIP output
Unchanged in substance:
evalSummaryStateandwriteInitialFIPReportwere skipped for any refined CpGrid, so CARFIN decks wrote all-zero summaries and a FIP report segfaulted on the never-populatedinplace_. The unsupported case is exactly a distributed refined grid (CollectDataOnIORank does not build its index maps there); serial is fine since the maps are the identity.Per review, the condition now lives in one private helper,
eclOutputEvalSupported_(), used at both call sites, so no future site can miss a condition.2.
compressedIndexForInteriorLGR: fatal vs −1, separatedPreviously any miss threw
std::out_of_rangefrommap::at(), and an exception here fires on some ranks and not others — a collective hazard during well setup. The rank-without-the-box case is reachable by construction: a rank holding no cell of a COMPDATL box (interior or overlap) still resolves every LGR well when building the well containers. (To answer the inline question directly: I hit it as a np≥6 deadlock during development, on a faulted-box variant ofSIMPLE_2PH_W_FAULT_LGR— that deck needs refinement across a fault, which CpGrid does not support yet, so the deck is not reproducible on master, but the code path is, from any COMPDATL deck whose box misses a rank.)Per review, misses are no longer treated alike:
compressedIndexForInterior's contract;checkAllConnectionsFoundstill verifies global existence collectively.3. The Cartesian→compressed map: level-zero cells only
Adopting the suggestion made inline: the map now only contains unrefined cells — "a mapping for existing cells on level 0 only" — and is cleared on rebuild. On a refined grid a level-zero Cartesian index is not a unique key (every child reports its ancestor's index), so inserting refined cells made the winner arbitrary. Now a refined-away level-zero index resolves to "not present" and flows into the existing cells-not-found handling instead of silently binding to an arbitrary child; cells inside a refinement are addressed only through the LGR-aware lookup.
The rebuild after refinement is still required — refinement renumbers the leaf, so the load-balance-time entries are stale (on
SPE1CASE1_CARFIN1,PRODresolved to cart 91 instead of its perforation cell 299; fixed: active index 923, first-iteration CNV matches the non-LGR baseline, serial ≡ parallel). The post-grid-change updates are factored intoupdateDerivedGridState_(), shared byloadBalance()andaddLgrs(), per review.Testing
Against opm-common/opm-grid master:
SPE1CASE1_CARFIN1(coarse wells, box refined): serial 51 Newton with non-zero summaries (all-zero before); np=2 with--enable-ecl-output=falsealso 51 Newton — serial ≡ parallel. Parallel with output enabled still fails inCollectDataOnIORank, as on master; that is the separate parallel-output work, not this PR.SPE1CASE1_CARFINat np=4, i.e. the configurationspe1case1_carfin_parallelruns in CI: completes, 60 Newton.SPE1CASE1_CARFIN_GR(COMPDATL well, whole grid refined — so the level-zero map is legitimately empty, which is what exercises the reworked LGR lookup): serial completes, 109 Newton. I did not get a np=2 run of this deck to complete — it sits in the first report step. Note CI registersspe1case1_carfin_grserially only, and whole-grid LGR in parallel looks unsupported independently of this PR, so I am not claiming it as evidence either way.SPE1CASE1(no LGR): 313 Newton, unchanged —hasFather()is false everywhere, so the map is identical.