LGR: rebuild Cartesian->compressed map after refinement, enable serial summary output, fix rank-asymmetric throw by hnil · Pull Request #7245 · OPM/opm-simulators

hnil · 2026-07-29T09:31:59Z

Reworked per review — thanks, the comments improved all three fixes. Same three bugs, different shapes:

1. Serial LGR summary/FIP output

Unchanged in substance: evalSummaryState and writeInitialFIPReport were skipped for any refined CpGrid, so CARFIN decks wrote all-zero summaries and a FIP report segfaulted on the never-populated inplace_. 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, separated

Previously any miss threw std::out_of_range from map::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 of SIMPLE_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:

LGR name unknown → fatal. Every rank registers every requested LGR (empty level grid where it holds no box cells — interior or overlap; the earlier comment's "rank-interior/owning rank" language was wrong and is gone), so the level structure is identical on all ranks and a failed name lookup is a programming error.
Connection (i,j,k) outside the LGR's dimensions → fatal, same reasoning (validated at parse time).
Cell absent from this rank's level mapper → −1, the one legitimate miss, mirroring compressedIndexForInterior's contract; checkAllConnectionsFound still 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, PROD resolved 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 into updateDerivedGridState_(), shared by loadBalance() and addLgrs(), 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=false also 51 Newton — serial ≡ parallel. Parallel with output enabled still fails in CollectDataOnIORank, as on master; that is the separate parallel-output work, not this PR.
SPE1CASE1_CARFIN at np=4, i.e. the configuration spe1case1_carfin_parallel runs 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 registers spe1case1_carfin_gr serially 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.

akva2

Looks fine to me. Small suggestions.

akva2 · 2026-07-29T11:14:57Z

+            // 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_();


Since this is shared with loadBalance(), I'd consider adding a separate method to avoid dupe and future problems like this?

I dont't think it is currently shared with loadbalance, but might be later.

it's not shared, it's copy-pasted. currently the only difference is the lack of the update, with the update added, they do exactly the same thing, just look at the code..

akva2 · 2026-07-29T11:21:10Z

        using GridType = std::remove_cv_t<std::remove_reference_t<decltype(grid)>>;
        constexpr bool isCpGrid = std::is_same_v<GridType, Dune::CpGrid>;
-        if (!isCpGrid || (grid.maxLevel() == 0)) {
+        if (!isCpGrid || (grid.maxLevel() == 0) || (grid.comm().size() == 1)) {


Likewise I would consider moving this c&p to a utility member to avoid missing conditions in the future.

hnil · 2026-07-29T12:41:03Z

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:

WellConnections: bound the Cartesian->compressed lookup opm-grid#1053 — stops LGR-local well positions from anchoring wells to unrelated coarse cells during load balancing. Relevant to the "known gap" noted above about the well's rank and the box's refining rank diverging.
Write INIT/restart LGR sections in EGRID grid order, and mirror integer maps onto LGR grids opm-common#5252 — writes the INIT/UNRST per-LGR sections in EGRID grid order. Commit 2 here is what makes serial LGR summary output get produced at all, so the two together are what make LGR output actually loadable in post-processors.

blattms

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.

blattms · 2026-07-30T08:39:06Z

+        // connection's cell is not interior on this rank.  In a parallel run
+        // the refinement is rank-interior - the box is refined only on the rank
+        // that owns it - so on every other rank the level grid for this LGR is


I think we need to correct this comment, unless I misunderstand "rank-interior" and "owns it". Cells in the overlap on the rank are refined, too

blattms · 2026-07-30T08:45:54Z

+            // 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_();


I dont't think it is currently shared with loadbalance, but might be later.

blattms · 2026-07-30T08:52:17Z

+            // 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_();


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.

blattms · 2026-07-30T08:56:42Z

+        if (levelIt == nameToLevel.end()) {
+            return -1;
+        }


I am wondering where you experienced this problem. If this happens then it should be fixed elsewhere.

Here this should be fatal.

blattms · 2026-07-30T08:58:59Z

+        const auto& mappers = ParentType::lgrMappers_.value();
+        if (lgr_level < 0 || static_cast<std::size_t>(lgr_level) >= mappers.size()) {
+            return -1;
+        }


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...

blattms · 2026-07-30T09:01:11Z

+        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
+        }


hnil · 2026-07-31T09:22:45Z

Points taken on all four - the review was right on each. Reworked and force-pushed:

The comment was wrong: overlap cells in the box are refined too. Corrected, and that also means a mapper hit can be an overlap copy, so the lookup now filters on interiority like compressedIndexForInterior (otherwise two ranks claim the same connection).
Unknown LGR name is now fatal: names are registered on every rank (empty level grid where the box is absent), so a miss can only be a programming error.
The level bounds check was dead; replaced by an assert.
The map is now level-zero-only as you suggested, which keeps it one-to-one; the rebuild after refinement stays because leaf indices shift.

Underlying bugs written up in #7260 and #7261. Verified: serial SPE1CASE1_CARFIN{,_GR} byte-identical to the previous version; SPE1CASE1_CARFIN on 2 ranks identical convergence. A well inside an LGR still fails on 2 ranks in beginReportStep - pre-existing and separate, noted in #7261.

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.

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).

hnil requested review from aritorto and arturcastiel July 29, 2026 09:32

hnil added manual:irrelevant This PR is a minor fix and should not appear in the manual manual:bugfix This PR is a bug fix and should be noted in the manual and removed manual:irrelevant This PR is a minor fix and should not appear in the manual labels Jul 29, 2026

hnil requested a review from akva2 July 29, 2026 11:11

akva2 approved these changes Jul 29, 2026

View reviewed changes

hnil mentioned this pull request Jul 29, 2026

WellConnections: bound the Cartesian->compressed lookup OPM/opm-grid#1053

Open

hnil mentioned this pull request Jul 29, 2026

Write INIT/restart LGR sections in EGRID grid order, and mirror integer maps onto LGR grids OPM/opm-common#5252

Merged

blattms requested changes Jul 30, 2026

View reviewed changes

hnil force-pushed the pr/lgr-serial-output-and-robustness branch from ac2b54e to add3473 Compare July 31, 2026 09:11

This was referenced Jul 31, 2026

Cartesian->compressed map is stale after LGR refinement #7260

Open

compressedIndexForInteriorLGR fails rank-asymmetrically in parallel LGR runs #7261

Open

hnil added 3 commits July 31, 2026 14:20

hnil force-pushed the pr/lgr-serial-output-and-robustness branch from add3473 to 76eb86a Compare July 31, 2026 14:00

hnil removed the request for review from arturcastiel August 2, 2026 17:29

Uh oh!

Conversation

hnil commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Serial LGR summary/FIP output

2. compressedIndexForInteriorLGR: fatal vs −1, separated

3. The Cartesian→compressed map: level-zero cells only

Testing

Uh oh!

akva2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hnil commented Jul 29, 2026

Uh oh!

blattms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hnil commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hnil commented Jul 29, 2026 •

edited

Loading

2. `compressedIndexForInteriorLGR`: fatal vs −1, separated

blattms left a comment •

edited

Loading