Map region arrays onto the leaf grid for LGR (EQLNUM, PVTNUM, SWATINIT, FIP) by hnil · Pull Request #7244 · OPM/opm-simulators

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

EQLNUM, PVTNUM, SWATINIT and the FIP region arrays are given on the unrefined input grid, but are consumed per leaf cell. With an LGR the leaf is larger and reordered, so the arrays were only resize()d — zero-padding the refined cells into region 0 and dropping them from the sums. LookUpData maps each leaf cell to its input-grid origin instead (a refined cell inherits its parent's value), which is the identity without LGRs.

Depends on #7245, and is not observable without it. On master all cell-based output is zero when an LGR is present, so this PR alone changes nothing you can measure.

Measured on lgr/SPE1CASE1_CARFIN1-3DCORNERPOINT_XYZ (which refines fine on upstream opm-grid), against the same deck with the CARFIN block removed:

	no LGR	LGR, #7245	LGR, #7245 + this
FPR	4992.268066	4992.047363	4992.268066
FOIP	8.513534e+07	8.393247e+07	8.513534e+07
FGIP	1.081219e+08	1.065942e+08	1.081219e+08
FRPV	1.832981e+08	1.807087e+08	1.832981e+08
FHPV	1.429725e+08	1.409528e+08	1.429725e+08

#7245 turns the output on; this PR makes it right. With both, refinement reproduces the unrefined field totals exactly — which is the property that should hold.

No LGR deck in opm-tests currently requests region FIP, and only one requests FPR, so nothing in CI covers this today.

blattms · 2026-08-11T12:17:45Z

jenkins build this please

blattms

Thanks. this is a big step in the right direction.

Unfortunately, it does not go far enough and leaves some containers (regions_) as before.

We should probably make sure that GenericOutputBlackoilModel::regions_ has the correct size from the beginning (number of cells on the leaf). This probably means that the creation of regions_ in is not done in the constructor of GenericOutputBlackoilModel (no knowledge of the grid), but in the constructor of OutputBlackOilModule.

blattms · 2026-08-11T12:22:52Z

+            const LookUpData<Grid, GridView> lookUpData(gridView);
+            const auto input =
+                lookUpData.assignFieldPropsDoubleOnLeaf(eclipseState.fieldProps(), "SWATINIT");
+            swatInit_.resize(input.size());
+            std::ranges::copy(input, swatInit_.begin());


This is too complicated.
Assignment should make sure that the size is correct anyway.
In addition which should skip the temporary and copy if Scalar is double like in the original code

Done - assign/move sizes it, and the double case no longer copies.

blattms · 2026-08-11T20:52:10Z

+            const LookUpData<Grid, GridView> lookUpData(gridView);
+            const auto input =
+                lookUpData.assignFieldPropsDoubleOnLeaf(eclipseState.fieldProps(), "SWATINIT");
+            swatInit_.resize(input.size());
+            std::ranges::copy(input, swatInit_.begin());
        }


Please skip the additional copy if Scalar is double like in the original code

Done, the if constexpr fast path is back.

blattms · 2026-08-12T05:36:24Z

-            this->createLocalRegion_(region_pair.second);
+            this->createLocalRegion_(region_pair.first, region_pair.second);


If we make sure in the base class that the vectors in this->regions_ have the correct then this can stay as before.

Done - back to createLocalRegion_(std::vector<int>&). LookUpData::operator() maps the array it is handed, so there is no need to re-read it from the field properties by name.

blattms · 2026-08-12T05:38:03Z

If we make sure in the base class that the vectors in this->regions_ have the correct then this can stay as before.
As is this just creates overhead.

blattms · 2026-08-12T05:41:21Z

-            // Note: We explicitly use decltype(auto) here because the
-            // default scheme (-> auto) will deduce an undesirable type.  We
-            // need the "reference to vector" semantics in this instance.
+            // The region arrays (PVTNUM, FIP regions) are given on the input
+            // grid, but RegionPhasePoreVolAverage indexes them by leaf cell.
+            // With LGRs the leaf has more cells, so map each onto the leaf (a
+            // refined cell inherits its parent cell's region; identity without
+            // LGRs) and let the lambda serve those. The map is captured by value
+            // so it outlives in the stored callable; decltype(auto) keeps the
+            // required "reference to vector" return semantics.
+            const LookUpData<Grid, GridView> lookUpData(this->simulator_.gridView());
+            std::map<std::string, std::vector<int>> leafRegions;
+            for (const auto& rsetName : rset) {
+                leafRegions[rsetName] = lookUpData.template assignFieldPropsIntOnLeaf<int>(
+                    this->eclState_.fieldProps(), rsetName, /*needsTranslation=*/false);
+            }
+


I don't see why we need to create this extra map. Please omit it.
It should suffice to make sure we return the containers on the leaf in the return statement of the lambda below.

Done - the extra map is gone. The FIP entries come straight from regions_, which createLocalRegion_ has already put on the leaf. Only PVTNUM keeps a copy: it is not a fluid-in-place region, and hnil would rather it stayed out of regions_ (which outputFipAndResvLog iterates whole).

blattms · 2026-08-12T07:25:21Z

-                         [fp = std::cref(this->eclState_.fieldProps())]
+                         [regions = std::move(leafRegions)]
                         (const std::string& rsetName) -> decltype(auto)
-                         { return fp.get().get_int(rsetName); });
+                         { return regions.at(rsetName); });


Considering my other comment only the capture and the return line should need changes here.

Capture and return line only now, as you suggested.

blattms · 2026-08-12T07:27:40Z

-        // For CpGrid with LGRs, where level zero grid has been distributed,
-        // resize region is needed, since in this case the total amount of
-        // element - per process - in level zero grid and leaf grid do not
-        // coincide, in general.
-        region.resize(simulator_.gridView().size(0));
+        // FIP region arrays (FIPNUM, ...) are given on the (unrefined) input grid,
+        // but the FIP/field sums run over the leaf grid. With LGRs the leaf has
+        // more cells than the input grid, so the array must be MAPPED onto the
+        // leaf - a refined cell inherits its parent cell's region. Previously this
+        // only resize()d the global array to the leaf size, which zero-padded the
+        // appended refined cells: every refined cell landed in region 0 and was
+        // dropped from the FIP/field sums, giving a wrong field pressure (FPR) and
+        // region FIP whenever an LGR was present. LookUpData performs the parent
+        // inheritance and is the identity without LGRs, so non-LGR runs are
+        // unchanged.
+        const LookUpData<Grid, GridView> lookUpData(simulator_.gridView());
+        region = lookUpData.template assignFieldPropsIntOnLeaf<int>(
+            this->eclState_.fieldProps(), name, /*needsTranslation=*/false);
+
+        // Exclude non-interior (overlap/ghost) cells from the region sums.


Assuming that my other comments make sense and are applied only the removed lines would be be part of the PR. That is because regions already has the correct size.

The mapping stayed in createLocalRegion_ rather than moving to the constructor: OutputCompositionalModule derives from the same base and calls it too, so moving the fill into OutputBlackOilModule alone would leave the compositional path unmapped. It now maps in place, so regions_ is leaf-correct before anything reads it, which I think gets you the same result.

blattms · 2026-08-13T16:57:58Z

-        // For CpGrid with LGRs, where level zero grid has been distributed,
-        // resize region is needed, since in this case the total amount of
-        // element - per process - in level zero grid and leaf grid do not
-        // coincide, in general.
-        region.resize(simulator_.gridView().size(0));
+        // The array arrives on the (unrefined) input grid, but the FIP/field sums
+        // run over the leaf.  With LGRs the leaf has more cells, so it must be
+        // MAPPED - a refined cell inherits its parent's region - not merely
+        // resize()d, which zero-padded the refined cells into region 0 and dropped
+        // them from the sums.  LookUpData is the identity without LGRs.
+        const LookUpData<Grid, GridView> lookUpData(simulator_.gridView());
+
+        std::vector<int> onLeaf(simulator_.gridView().size(0), 0);
        std::size_t elemIdx = 0;
        for (const auto& elem : elements(simulator_.gridView())) {
-            if (elem.partitionType() != Dune::InteriorEntity) {
-                region[elemIdx] = 0;
+            // Non-interior (overlap/ghost) cells stay out of the region sums.
+            if (elem.partitionType() == Dune::InteriorEntity) {
+                onLeaf[elemIdx] = lookUpData(elem, region);
            }

            ++elemIdx;
        }
+
+        region = std::move(onLeaf);


Sorry, this is still not what I asked for.
We should make sure that regions_ has the correct size in lime 178 already. Then we do not need any temporary containers here and just need to change existing entries.

Done properly this time - the mapping happens once in the constructor, before the createLocalRegion_ loop, so regions_ already has the leaf size when anything looks at it. createLocalRegion_ is back to masking non-interior cells and nothing else: its diff against master is now only the removed resize(), which is what you asked for.

Equilibration read EQLNUM, PVTNUM and SWATINIT straight from the input-grid field properties and indexed them by leaf cell. Without LGRs the leaf coincides with the input grid, so this is correct. With an LGR the leaf has more cells and a different ordering, so: - EQLNUM: equilnum() copied the input-grid array into a leaf-sized vector with std::ranges::transform, misaligning coarse cells and leaving every refined cell in equil region 1. Refined cells then used the wrong OWC, raising the water-oil contact (the reported symptom: LGR initialized far too wet, mobile water mean ~0.675 vs ~0.567 in the reference; non-LGR matched). - PVTNUM: setRegionPvtIdx() indexed the input-grid PVTNUM by a leaf cell index (out of bounds for refined cells -> garbage PVT region -> 'table has 0 sampling points' once a region's first cell was refined). - SWATINIT: copied input-grid values into a leaf-indexed vector. Map each through LookUpData::assignFieldProps{Int,Double}OnLeaf, so a refined cell inherits its parent cell's value. This is the identity for unrefined grids, so non-LGR cases are byte-identical; equilnum now takes the GridView (LookUpData needs it). Verified: model2_lgr TEST_LGR_INJ1_NOTHPRES initial SWAT now matches the reference within ~2% per cell - the same OPM-vs-reference spread as the non-LGR case (was a gross WOC error before). No regression: non-LGR, single-level LGR (SPE1CASE1_CARFIN1) and nested LGR (serial + np=2) still equilibrate and run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…tc.) createLocalRegion_ built each FIP region array (FIPNUM, ...) from the input-grid field property and then only resize()d it to the leaf cell count. std::vector resize keeps the existing (input-grid) values and zero-pads the appended entries, so with an LGR every refined leaf cell got region 0 and was dropped from the FIP/field region sums. The field pressure FPR (and FPRP, region FIP) are computed as the FIPNUM-region sum, so excluding the ~10k refined cells (near the well, different pressure) made FPR wrong by ~4% while per-cell maps stayed correct. Map the region array onto the leaf with LookUpData::assignFieldPropsIntOnLeaf so a refined cell inherits its parent cell's region (identity without LGRs, so non-LGR runs are unchanged), then keep the existing non-interior -> region 0 masking. Verified on model2_lgr: FPR vs the reference improves from abs 9.06 to 0.055 (matching the non-LGR baseline ~0.12), serial and at np=2; nolgr FPR unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The PPO/PPG/PPW and RPP* datum-pressure summary keywords build a RegionPhasePoreVolAverage from PVTNUM/FIP region arrays read on the input grid, then index them by leaf cell. With LGRs the leaf is larger, so refined cells read past the end of the input-grid arrays (out of bounds / wrong region). Map each region array onto the leaf via LookUpData (refined cell inherits its parent's region; identity without LGRs) and capture the mapped arrays by value in the lookup lambda so they outlive in the stored callable. Verified: an LGR deck with RPPO now runs; black-oil non-LGR output unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…il only) These extension initial conditions (SSOL, SPOLY, SPOLYMW, SMICR, SBIOF, SOXYG, SUREA, SCALC) are read from the input-grid field properties and indexed by leaf cell without mapping refined cells to their parent, so they would be wrong/out of bounds under LGR. LGR is supported for black-oil only; fail early with a clear message when an LGR/CARFIN deck also enables one of these extensions, instead of producing silently wrong results. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…st path Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

createLocalRegion_ takes its vector again instead of a name: LookUpData's operator() maps the array it is already given, so there is no need to re-read it from the field properties. The averaging lambda now serves the FIP entries straight from regions_, which createLocalRegion_ has already put on the leaf. PVTNUM is not a fluid-in-place region and stays out of regions_, so it keeps its own leaf-mapped copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

hnil · 2026-08-21T08:02:09Z

jenkins build this please

Moves the mapping out of createLocalRegion_ and into a single pass in the constructor, so regions_ already has the leaf size when anything looks at it. createLocalRegion_ then only masks the non-interior cells, as before - its diff against master is now just the removed resize(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

bska · 2026-08-27T11:28:46Z

jenkins build this please

Copilot

Pull request overview

This PR fixes region/field-property arrays that are defined on the unrefined input grid (e.g., EQLNUM, PVTNUM, SWATINIT, and FIP region arrays) but are consumed indexed by leaf cells. With LGR/CARFIN refinement the leaf grid has more cells and a different ordering, so the previous “resize to leaf” approach effectively zero-padded refined cells and produced incorrect totals; the new logic maps each leaf cell back to its input-grid origin via LookUpData (refined cells inherit their parent’s value), preserving correct region assignments with and without LGRs.

Changes:

Map FIP region arrays onto the leaf grid in the black-oil output module before downstream consumers index them.
Map EQLNUM, SWATINIT, and PVTNUM onto the leaf grid in equilibration/initialization code paths using LookUpData.
Fail fast for LGR combined with extensions whose initial-condition reads are not leaf-mapped (solvent/polymer/bioeffects/MICP).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
opm/simulators/flow/OutputBlackoilModule.hpp	Map region arrays (and `PVTNUM` for averaging) onto leaf cells for correct LGR summaries/FIP handling.
opm/simulators/flow/FlowGenericProblem_impl.hpp	Add an early error when LGR is used with extensions that are not leaf-mapped.
opm/simulators/flow/equil/InitStateEquil.hpp	Update `setRegionPvtIdx` signature to accept `GridView` for leaf-mapped `PVTNUM`.
opm/simulators/flow/equil/InitStateEquil_impl.hpp	Use `LookUpData` to leaf-map `EQLNUM`, `SWATINIT`, and `PVTNUM` for LGR correctness.

Suppressed comments (1)

opm/simulators/flow/OutputBlackoilModule.hpp:945

createLocalRegion_() also indexes the region array by a monotonically incremented counter rather than the element’s leaf index. Using indexSet().index(elem) keeps the intent clear and avoids coupling correctness to iteration order.

    void createLocalRegion_(std::vector<int>& region)
    {
        std::size_t elemIdx = 0;
        for (const auto& elem : elements(simulator_.gridView())) {
            if (elem.partitionType() != Dune::InteriorEntity) {
                region[elemIdx] = 0;
            }

            ++elemIdx;
        }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+        for (auto& [name, region] : this->regions_) {
+            std::size_t elemIdx = 0;
+            for (const auto& elem : elements(simulator_.gridView())) {
+                onLeaf[elemIdx++] = lookUpData(elem, region);
+            }
+
+            region = onLeaf;
+        }


This has been blocked for too long with no response from original reviewer.

bska

As far as I can tell, all concerns have been addressed. I'm merging this into the master branch now. Any additional concerns can be addressed in follow-up work.

hnil requested review from akva2 and arturcastiel July 29, 2026 09:31

hnil added the manual:bugfix This PR is a bug fix and should be noted in the manual label Jul 29, 2026

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

hnil force-pushed the pr/lgr-region-arrays-on-leaf branch 2 times, most recently from 5d2f024 to e4c4a8d Compare August 11, 2026 12:06

blattms requested changes Aug 12, 2026

View reviewed changes

This was referenced Aug 12, 2026

LGR: rebuild Cartesian->compressed map after refinement, enable serial summary output, fix rank-asymmetric throw #7245

Open

tests: field totals must not change when part of the grid is refined #7315

Draft

blattms reviewed Aug 13, 2026

View reviewed changes

blattms previously requested changes Aug 13, 2026

View reviewed changes

hnil and others added 6 commits August 19, 2026 09:42

Address review: let assignment size swatInit_, and keep the double fa…

adf32ae

…st path Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

hnil force-pushed the pr/lgr-region-arrays-on-leaf branch from b3f55f8 to 7550dde Compare August 19, 2026 07:42

bska requested a lite review from Copilot August 27, 2026 11:28

Copilot started reviewing on behalf of bska August 27, 2026 11:29 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

Comment thread opm/simulators/flow/OutputBlackoilModule.hpp

Comment on lines +926 to +933

for (auto& [name, region] : this->regions_) {

std::size_t elemIdx = 0;

for (const auto& elem : elements(simulator_.gridView())) {

onLeaf[elemIdx++] = lookUpData(elem, region);

}

region = onLeaf;

}

bska approved these changes Sep 3, 2026

View reviewed changes

bska merged commit 8856721 into OPM:master Sep 3, 2026
2 checks passed

		this->createLocalRegion_(region_pair.second);
		this->createLocalRegion_(region_pair.first, region_pair.second);

Uh oh!

Conversation

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

Uh oh!

Uh oh!

blattms commented Aug 11, 2026

Uh oh!

blattms 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!

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!

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 Aug 21, 2026

Uh oh!

bska commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

bska left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hnil commented Jul 29, 2026 •

edited

Loading