Reservoir coupling: Communicate group potentials from slaves and compute guide rates #6170

hakonhagland · 2025-04-11T06:53:39Z

Sends group potentials from the slave processes to the master process, then use those potentials in the master process to calculate guide rates.

A future PR will calculate group target in the master based on the guide rates and send those targets to the slave groups.

hakonhagland · 2025-04-11T06:55:41Z

I will run jenkins from the opm-common PR later

Copilot

Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

CMakeLists_files.cmake: Language not supported

opm/simulators/flow/ReservoirCouplingSlave.cpp

blattms

Thanks a lot for the contribution. A few comments for now.

Note that this does not build without MPI.

blattms · 2025-04-16T09:54:19Z

opm/simulators/flow/ReservoirCouplingSlave.cpp

+ReservoirCouplingSlave::
+serializePotentials_(const std::vector<Potentials>& potentials) const
+{
+    std::vector<double> pot_vec;


mabe reserve the necessary space?

opm/simulators/flow/ReservoirCouplingSlave.cpp

opm/simulators/flow/ReservoirCouplingMaster.hpp

opm/simulators/flow/ReservoirCouplingMaster.cpp

opm/simulators/flow/ReservoirCouplingSpawnSlaves.cpp

blattms · 2025-04-16T11:36:28Z

opm/simulators/flow/ReservoirCouplingSpawnSlaves.cpp

+    auto num_slaves = this->master_.numSlavesStarted();
+    auto master_groups = this->rescoup_.masterGroups();


Are you copying these intentionally? Maybe use const references instead?

opm/simulators/wells/BlackoilWellModel_impl.hpp

blattms · 2025-04-28T08:08:43Z

Unfortunately, this needs to be rebased on master it seems.

hakonhagland · 2025-04-28T15:15:48Z

@blattms Thanks for the review with useful suggestions. I have added some commits to the PR to address the comments. I also added one commit to dump all guide rates to the debug file.

blattms · 2025-06-03T13:53:59Z

I am sorry for the delay. Now this needs another rebase.

blattms

Thanks a lot for rebasing.
This is looks quite good already.

My main reservation is the introduction of a reference to the simulator in the time stepping. I would hope that there is another way.

blattms · 2025-06-10T11:18:39Z

opm/simulators/wells/BlackoilWellModel_impl.hpp

+#ifdef RESERVOIR_COUPLING_ENABLED
+        if (this->isReservoirCouplingMaster()) {
+            this->guide_rate_handler_.receiveMasterGroupPotentialsFromSlaves();
+        }
+#endif
        //update guide rates
-        const auto& comm = simulator_.vanguard().grid().comm();
-        std::vector<Scalar> pot(this->numPhases(), 0.0);
-        const Group& fieldGroup = this->schedule().getGroup("FIELD", reportStepIdx);
-        WellGroupHelpers<Scalar>::updateGuideRates(fieldGroup,
-                                                   this->schedule(),
-                                                   this->summaryState(),
-                                                   this->phase_usage_,
-                                                   reportStepIdx,
-                                                   simulationTime,
-                                                   this->wellState(),
-                                                   this->groupState(),
-                                                   comm,
-                                                   &this->guideRate_,
-                                                   pot,
-                                                   local_deferredLogger);
+        this->guide_rate_handler_.updateGuideRates(
+            reportStepIdx, simulationTime, this->wellState(), this->groupState()
+        );
+#ifdef RESERVOIR_COUPLING_ENABLED
+        if (this->isReservoirCouplingSlave()) {
+            this->guide_rate_handler_.sendSlaveGroupPotentialsToMaster();
+        }
+#endif


This is probably a silly question: Why don't we do the receive and sends in GuideRateHandler::updateGuideRates?

Good question. Since updateGuideRates() is not directly dependent on sending slave group potentials and updating master group potentials. Rather, it will compute the potentials from the well potentials and store those values in the GuideRate object (for non-master groups), and for master groups it will retrieve the group potentials stored in the ReservoirCouplingMaster object to compute guide rates. See updateGuideRates() in updateWellControlsAndNetworkIteration() for an example where sending slave group potentials/receiving master group potentials is not done. So the slave group potentials only need to be communicated at the start of the timestep and no update of those will be done during the nonlinear iterations.

blattms · 2025-06-10T11:22:00Z

opm/simulators/wells/BlackoilWellModel_impl.hpp

+            this->guide_rate_handler_.updateGuideRates(
+                reportStepIdx, simulationTime, this->wellState(), this->groupState()
+            );


Maybe connected to the other comment:
There is no communication here. Why is this not needed? Please add a comment about this.

Good observation. I tried to explain this in the other comment. Also added a comment in the source code, see latest commit.

blattms · 2025-06-10T13:45:22Z

opm/simulators/flow/ReservoirCoupling.hpp

+    // Define a num_fields constant to be used in MPI communication to simplify serialization
+    // and deserialization of the struct. Note that sizeof(Potentials)/sizeof(double) is not
+    // guaranteed to be equal to num_fields due to padding.
+    static constexpr std::size_t num_fields = 3;


This does not seem to be used. I do not really get the comment. What is it for?

Yes the comment was not clear. It was used by the ReservoirCouplingMpiTraits.hpp to determine the block lengths for the MPI trait for the Potentials struct. However, to avoid duplication of code when developing MPI traits for other structs in the future, I decided to make the code more generic. Luckily, as a by-product of that change, the need for the num_fields variable was also eliminated. See the added commit for details.

opm/simulators/timestepping/AdaptiveTimeStepping.hpp

blattms · 2025-06-10T14:09:03Z

jenkins build this serial please

blattms · 2025-06-10T14:13:30Z

🙈 jenkins will fail of course. Handled by opm-common PR, already

hakonhagland · 2025-06-12T08:39:50Z

My main reservation is the introduction of a reference to the simulator in the time stepping.

@blattms Thanks for the detailed review. I have added some commits to address the comments you posed. I am also planning to add a commit to address the comment from @bska in the companion PR in opm-common. I think we can avoid getting the slave group potentials from the GuideRate object by storing them in the GroupState object instead. This will for now cause duplication of group potentials in GuideRate and GroupState. A future PR might try to remove that duplication by removing the potentials from the GuideRate object's internal state. What do you think? Then we can simply close/delete OPM/opm-common#4567.

hakonhagland · 2025-06-12T17:59:22Z

jenkins build this please

hakonhagland · 2025-06-13T05:36:59Z

@blattms and @bska The last commit tries to address OPM/opm-common#4567 (comment). Then OPM/opm-common#4629 will replace OPM/opm-common#4567. I have run jenkins successfully for both mpi and serial in OPM/opm-common#4629.

hakonhagland · 2025-06-17T12:40:33Z

jenkins build this please

hakonhagland · 2025-06-17T12:41:39Z

jenkins build this please

hakonhagland · 2025-06-17T13:57:30Z

jenkins build this please

hakonhagland · 2025-06-17T15:39:26Z

jenkins build this please

hakonhagland · 2025-06-17T16:12:16Z

jenkins build this serial please

hakonhagland · 2025-06-18T04:54:06Z

The last commit tries to address OPM/opm-common#4567 (comment). Then OPM/opm-common#4629 will replace OPM/opm-common#4567.

@blattms and @bska After review by @bska, I think the companion PR OPM/opm-common#4629 can also be eliminated. I have added a commit here as a replacement, and plan to close both opm-common PRs as they are no longer needed. Jenkins builds with and without serial succeed. Let me know if anything else needs to be added to this PR.

blattms · 2025-06-18T06:50:41Z

Thanks a lot. I'll take another look.

blattms

Overall this looks good. Just a few curious questions.

Personally, I would appreciate a bit more doxygen documentation for new classes.

opm/simulators/wells/GroupState.cpp

opm/simulators/timestepping/AdaptiveTimeStepping.hpp

opm/simulators/flow/ReservoirCouplingMpiTraits.hpp

blattms · 2025-06-20T13:51:38Z

Seems like I was too slow again and another Merge conflict appeared. I am really sorry.

Send group potentials from the slaves to the master process, then use the potentials in the master process to calculate guide rates.

Added debugging code to dump all guide rates

Even if a group is a pure injection group, we still need to include its group potential in the calculation of the production potential of a higher group

Using a custom MPI datatype for the potentials, we can avoid serialization and deserialization code.

Add generic code for building MPI traits for structs

Add a comment to improve readability

Add a comment to clarify that communication of slave group potentials is not done during the nonlinear iterations

Store the group potentials in the GroupState object so we avoid recomputing them later when we need to send the slave group potentials to the master.

Avoid reusing implementation detail itemName() of class GuideRateValue

Fix bug where emplace() did not update existing keys in the map. Instead use try_emplace() for efficient conditional construction and assignment.

Remove Simulator alias since it is not used by any code

Added Doxygen documentation for the GuideRateHandler class.

hakonhagland · 2025-06-23T06:04:41Z

jenkins build this please

hakonhagland · 2025-06-23T06:17:07Z

I would appreciate a bit more doxygen documentation for new classes.

@blattms Good idea. I have added a commit that adds documentation for the GuideRateHandler class.

hakonhagland · 2025-06-23T06:50:49Z

jenkins build this serial please

hakonhagland mentioned this pull request Apr 11, 2025

Return group/well potentials from GuideRate object OPM/opm-common#4567

Closed

hakonhagland force-pushed the send_pot4 branch from 1eaa060 to d493c36 Compare April 11, 2025 06:59

hakonhagland requested a review from Copilot April 11, 2025 08:45

Copilot AI reviewed Apr 11, 2025

View reviewed changes

opm/simulators/flow/ReservoirCouplingSlave.cpp Show resolved Hide resolved

opm/simulators/flow/ReservoirCouplingSlave.cpp Show resolved Hide resolved

hakonhagland force-pushed the send_pot4 branch 2 times, most recently from 8b5a02c to 59daa3d Compare April 11, 2025 14:18

blattms reviewed Apr 16, 2025

View reviewed changes

hakonhagland force-pushed the send_pot4 branch from 26f1316 to b996c7d Compare April 25, 2025 05:02

hakonhagland force-pushed the send_pot4 branch 2 times, most recently from 8183220 to 51198a8 Compare April 28, 2025 13:13

hakonhagland added the manual:irrelevant This PR is a minor fix and should not appear in the manual label May 9, 2025

hakonhagland force-pushed the send_pot4 branch from 51198a8 to 9bfcc29 Compare June 3, 2025 15:16

blattms reviewed Jun 10, 2025

View reviewed changes

hakonhagland force-pushed the send_pot4 branch 2 times, most recently from 1cbea70 to c03f2a2 Compare June 11, 2025 15:45

hakonhagland mentioned this pull request Jun 12, 2025

Make itemName() of GuideRateValue public OPM/opm-common#4629

Closed

hakonhagland force-pushed the send_pot4 branch from 5197391 to bad00c8 Compare June 17, 2025 12:41

hakonhagland force-pushed the send_pot4 branch from bad00c8 to 7ab228e Compare June 17, 2025 13:57

blattms reviewed Jun 20, 2025

View reviewed changes

opm/simulators/wells/GroupState.cpp Outdated Show resolved Hide resolved

opm/simulators/timestepping/AdaptiveTimeStepping.hpp Outdated Show resolved Hide resolved

opm/simulators/flow/ReservoirCouplingMpiTraits.hpp Show resolved Hide resolved

hakonhagland added 19 commits June 22, 2025 10:57

Communicate group potentials

827cda4

Send group potentials from the slaves to the master process, then use the potentials in the master process to calculate guide rates.

Debugging code to dump guide rates

006be80

Added debugging code to dump all guide rates

Fixed bug in guide rate calculation

5abe7d6

Even if a group is a pure injection group, we still need to include its group potential in the calculation of the production potential of a higher group

Fix serial build

cb031cd

Use MPI custom datatype for potentials

ce20b0a

Using a custom MPI datatype for the potentials, we can avoid serialization and deserialization code.

Use references instead of copying

3d54ad6

Remove redundant if check

9dcd882

Add Doxygen comment

a2ce7da

Rename method

439ea3f

Don't store simulator reference

55313a6

Generic code for building MPI traits

edf0493

Add generic code for building MPI traits for structs

Add a comment

9a2b368

Add a comment to improve readability

Add comment for clarification

1623f37

Add a comment to clarify that communication of slave group potentials is not done during the nonlinear iterations

Store group potentials in GroupState

6bd6c7a

Store the group potentials in the GroupState object so we avoid recomputing them later when we need to send the slave group potentials to the master.

Create guiderate item names for debugging

aee9658

Avoid reusing implementation detail itemName() of class GuideRateValue

Use try_emplace() instead of emplace()

61e5a3d

Fix bug where emplace() did not update existing keys in the map. Instead use try_emplace() for efficient conditional construction and assignment.

Remove Simulator alias

25ba941

Remove Simulator alias since it is not used by any code

Handle scalar/enum case correctly

b3334cc

Added documentation for GuideRateHandler class

22c6160

Added Doxygen documentation for the GuideRateHandler class.

hakonhagland force-pushed the send_pot4 branch from 9823ebc to 22c6160 Compare June 23, 2025 06:04

blattms merged commit a032b7a into OPM:master Jun 23, 2025
2 checks passed

		auto num_slaves = this->master_.numSlavesStarted();
		auto master_groups = this->rescoup_.masterGroups();

Reservoir coupling: Communicate group potentials from slaves and compute guide rates #6170

Reservoir coupling: Communicate group potentials from slaves and compute guide rates #6170

Uh oh!

Conversation

hakonhagland commented Apr 11, 2025

Uh oh!

hakonhagland commented Apr 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

blattms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blattms commented Apr 28, 2025

Uh oh!

hakonhagland commented Apr 28, 2025

Uh oh!

blattms commented Jun 3, 2025

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!

Uh oh!

blattms commented Jun 10, 2025

Uh oh!

blattms commented Jun 10, 2025

Uh oh!

hakonhagland commented Jun 12, 2025

Uh oh!

hakonhagland commented Jun 12, 2025

Uh oh!

hakonhagland commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hakonhagland commented Jun 17, 2025

Uh oh!

hakonhagland commented Jun 17, 2025

Uh oh!

hakonhagland commented Jun 17, 2025

Uh oh!

hakonhagland commented Jun 17, 2025

Uh oh!

hakonhagland commented Jun 17, 2025

Uh oh!

hakonhagland commented Jun 18, 2025

Uh oh!

blattms commented Jun 18, 2025

Uh oh!

blattms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

blattms left a comment •

edited

Loading

hakonhagland commented Jun 13, 2025 •

edited

Loading