Fix NONE production control reset for RC master groups by hakonhagland · Pull Request #6922 · OPM/opm-simulators

hakonhagland · 2026-03-11T07:57:56Z

Builds on #6918.

Problem

PR #6596 introduced updateNONEProductionGroups() to fix GMCTP/FMCTP summary output by resetting production control to NONE for groups not actively targeted by any well. On an RC master (which has no local wells), this incorrectly resets all groups (including FIELD and the entire master group hierarchy) to NONE after the first timestep.

At the next report step, calculateGroupConstraintRecursive_() walks up the hierarchy from master group to FIELD. When FIELD has NONE control in the group state, the recursion returns nullopt, and the fallback calls getProductionGroupTargetForMode(B1_M, NONE) which crashes with "Invalid Group::ProductionCMode".

Fix

Move updateNONEProductionGroups() from BlackoilWellModelGeneric to GroupStateHelper, which has access to the rescoup_ proxy
Add RC master hierarchy exception: groups that are master groups or their ancestors up to FIELD are excluded from the NONE reset, since they actively distribute targets to slave groups via the guide-rate hierarchy

hakonhagland · 2026-03-11T07:58:35Z

Putting this in draft mode until #6918 has been merged

hakonhagland · 2026-03-11T07:58:49Z

jenkins build this serial please

hakonhagland · 2026-03-11T10:48:41Z

jenkins build this serial please

hakonhagland · 2026-03-11T12:14:42Z

jenkins build this serial please

hakonhagland · 2026-03-13T08:06:58Z

jenkins build this serial please

GitPaean · 2026-03-13T08:38:56Z

Thanks for the invitation. I will review the PR during the day.

GitPaean · 2026-03-13T10:12:56Z

Do we have any test case to demonstrate the scenario that is targeted by this PR? Thanks.

totto82 · 2026-03-13T10:22:26Z

Do we have any test case to demonstrate the scenario that is targeted by this PR? Thanks.

I think the most important part of the review is to make sure we dont loose anything for the non-reservoir coupling cases. I.e., do we have sufficient tests for the NONE output code you implemented to be confident that this PR won't break anything?

The reservoir coupling is still WIP, with functionality added piece by piece. I can review the reservoir-coupling part of the PR, if you could double-check that the PR doesn't regress functionality we have for NONE summary output.

GitPaean · 2026-03-13T10:26:55Z

Do we have any test case to demonstrate the scenario that is targeted by this PR? Thanks.

I think the most important part of the review is to make sure we dont loose anything for the non-reservoir coupling cases. I.e., do we have sufficient tests for the NONE output code you implemented to be confident that this PR won't break anything?

The reservoir coupling is still WIP, with functionality added piece by piece. I can review the reservoir-coupling part of the PR, if you could double-check that the PR doesn't regress functionality we have for NONE summary output.

That is okay. If there is a small case, I would like to understand more to give suggestions. For examples, are the targets from the RC master side always active (means the slaves sides are always under the control for the RC master side).

NONE is not only for summary output, it will change the bahavoir for the following time steps.

If there is no small test cases available, that is also okay. I was thinking to suggest to ensure consistent logic all the way.

GitPaean

It looks fine for me and only a few very tiny comments. I leave the reservoir coupling part to others who are more familiar with the matter.

GitPaean · 2026-03-13T10:28:02Z

opm/simulators/wells/GroupStateHelper.cpp

+                targeted.insert(ws.group_target->group_name);
+            } else {
+                OPM_DEFLOG_THROW(std::runtime_error,
+                    "Well " + ws.name


Unless we are reversing the usage of fmt, I do not think it is necessary to change back to string concatenation.

GitPaean · 2026-03-13T10:38:30Z

opm/simulators/wells/GroupStateHelper.cpp

+    std::vector<int> production_control_used;
+    production_control_used.reserve(num_gpc);
+
+    for (const auto& [name, control] : prod_group_controls) {


control is not used, possibly to add [[maybe_unused]] . it did not give a warning, but it is better for readability from my side.

GitPaean · 2026-03-13T10:48:12Z

opm/simulators/wells/GroupStateHelper.cpp

+GroupStateHelper<Scalar, IndexTraits>::
+collectTargetedProductionGroups_() const
+{
+    std::unordered_set<std::string> targeted;


It might not matter much for most of the scenarios. There was a "nice-to-have" reserve to allocate space to avoid reallocation.

totto82 · 2026-03-13T11:15:52Z

The reservoir coupling part is fine. This can be merged after @GitPaean comments are addressed.

hakonhagland · 2026-03-13T17:37:51Z

Thanks for the review @totto82 and @GitPaean.

@GitPaean I've pushed fixes for your three inline comments (fmt::format, unused binding, reserve).

Regarding your questions:

Test case: The test data files are at opm-tests/rescoup/rc_m1/master, specifically RC-01_MAST_PRED.DATA, a small prediction case where the master model has one grid cell and no wells, and controls production/injection from two slave models. RC test cases require multiple executables running via MPI, so they can't be set up as standard regression tests yet. Proper regression tests will probably be added once the RC feature is complete.

Are RC master targets always active? Not necessarily — the master group may not control any production wells on the slave side (e.g., if the slave has only injectors, or uses history-matching wells). Determining true activity would require the slave to communicate its well activity status back to the master via additional MPI communication. This is noted as a TODO in the code (GroupStateHelper.cpp:2128-2130). For now, the fix conservatively excludes all master hierarchy groups from the NONE reset, which is safe: keeping a group at its scheduled control (e.g., ORAT) when it has no active wells is harmless (target is effectively zero), while incorrectly resetting to NONE causes crashes in calculateGroupConstraintRecursive_().

NONE beyond summary output: Good point. I agree that NONE is not just for summary output. It acts as an "inherit from parent" marker that affects constraint checking, guide rate distribution, gas lift optimization, and well rate scaling, and it persists across timesteps until a schedule event resets it. This is why the fix is important: when updateNONEProductionGroups() incorrectly resets FIELD to NONE on the RC master, the constraint calculator can no longer find a valid controlling group when walking up the hierarchy, which causes the crash.

Non-RC regression safety: The new RC-specific code path is guarded by #ifdef RESERVOIR_COUPLING_ENABLED, so non-RC builds behave identically to before. The refactoring (moving the function from BlackoilWellModelGeneric to GroupStateHelper) preserves the existing logic unchanged.

I can squash the review fix-up commits into the original before merging if you prefer a clean single commit. Let me know!

hakonhagland · 2026-03-13T17:38:29Z

jenkins build this serial please

hakonhagland · 2026-03-13T18:18:07Z

jenkins build this serial please

GitPaean · 2026-03-13T22:45:29Z

I can squash the review fix-up commits into the original before merging if you prefer a clean single commit. Let me know!

I think this will be nice.

A group control mode is NONE basically means there are other constraints (group or well level) are more restrictive, this group's constraints are not limiting anything. or in other words the subgroups are under producing this group's target or constraints. From your comments, I think it might also happen to RC master group level, then the logic might need to be extended to there.

PR OPM#6596 introduced updateNONEProductionGroups() which resets production control to NONE for groups not targeted by any well. On an RC master (no local wells), this incorrectly resets ALL groups including FIELD and the master group hierarchy, causing a crash in getProductionGroupTargetForMode() at the next report step when the constraint calculator encounters NONE control on FIELD. Move updateNONEProductionGroups() from BlackoilWellModelGeneric to GroupStateHelper, which has access to the rescoup_ proxy. Add an exception for RC master hierarchy groups (master groups and their ancestors up to FIELD) that actively distribute targets to slave groups via the guide-rate hierarchy.

hakonhagland · 2026-03-14T05:07:38Z

jenkins build this serial please

hakonhagland · 2026-03-14T05:11:07Z

I can squash the review fix-up commits into the original before merging if you prefer a clean single commit.

@GitPaean Done!

hakonhagland added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Mar 11, 2026

hakonhagland marked this pull request as draft March 11, 2026 07:58

hakonhagland force-pushed the fix_mg_none_ctrl branch from dcc9ea8 to 1eed4ab Compare March 11, 2026 10:48

hakonhagland force-pushed the fix_mg_none_ctrl branch from 1eed4ab to fe89d7e Compare March 11, 2026 12:14

hakonhagland requested a review from GitPaean March 12, 2026 04:44

hakonhagland mentioned this pull request Mar 12, 2026

Group private methods in GroupStateHelper header #6931

Draft

hakonhagland force-pushed the fix_mg_none_ctrl branch from fe89d7e to 7f6817b Compare March 13, 2026 08:06

hakonhagland marked this pull request as ready for review March 13, 2026 08:06

GitPaean reviewed Mar 13, 2026

View reviewed changes

hakonhagland force-pushed the fix_mg_none_ctrl branch from ce601ff to 92fa64a Compare March 14, 2026 05:07

Conversation

hakonhagland commented Mar 11, 2026

Problem

Fix

Uh oh!

hakonhagland commented Mar 11, 2026

Uh oh!

hakonhagland commented Mar 11, 2026

Uh oh!

hakonhagland commented Mar 11, 2026

Uh oh!

hakonhagland commented Mar 11, 2026

Uh oh!

hakonhagland commented Mar 13, 2026

Uh oh!

GitPaean commented Mar 13, 2026

Uh oh!

GitPaean commented Mar 13, 2026

Uh oh!

totto82 commented Mar 13, 2026

Uh oh!

GitPaean commented Mar 13, 2026

Uh oh!

GitPaean left a comment

Choose a reason for hiding this comment

Uh oh!

GitPaean Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

GitPaean Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

GitPaean Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

totto82 commented Mar 13, 2026

Uh oh!

hakonhagland commented Mar 13, 2026

Uh oh!

hakonhagland commented Mar 13, 2026

Uh oh!

hakonhagland commented Mar 13, 2026

Uh oh!

GitPaean commented Mar 13, 2026

Uh oh!

hakonhagland commented Mar 14, 2026

Uh oh!

hakonhagland commented Mar 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants