When checking for zero target rates, also check wells under group control where required #5232

steink · 2024-02-28T15:17:56Z

Several places in the code we check whether a well is set to inject/produce at zero rate since this case requires special treatment. This is currently done by the function wellUnderZeroRateTarget (often called from stopppedOrZeroRateTarget). Wells under group control are currently not included in this check, although the special treatments are equally required if a well is assigned zero rate from a group.

I've written draft versions of the two above mentioned functions that also take group-controlled wells into account. Would just like some feedback/discussion whether this seems reasonable before I go ahead and suggest updating all relevant function calls (the new functions have temporary names *Version).

I've verified that this approach fixes an issue with iterateWellEqWithSwitching which currently does not behave nicely for wells that get assigned zero rate from group.

GitPaean · 2024-02-29T14:25:36Z

The code probably works while will come with suggestion for refactoring later.

GitPaean · 2024-02-29T14:25:43Z

jenkins build this please

opm/simulators/wells/WellInterface_impl.hpp

steink · 2024-03-19T12:06:17Z

opm/simulators/wells/MultisegmentWell_impl.hpp

@@ -159,7 +159,7 @@ namespace Opm
                           const WellState& well_state,
                           DeferredLogger& /* deferred_logger */)
    {
-        const bool stop_or_zero_rate_target = this->stopppedOrZeroRateTarget(summary_state, well_state);
+        const bool stop_or_zero_rate_target = this->stoppedOrZeroRateTargetIndividual(summary_state, well_state);


I don't think setting the WQTotal of first segment explicitly to zero for zero-target wells is strictly necessary below, so have left out the checking for zero targets from groups here.

It can be debatable. If we want to be a little more systematical, we probably want to remove this stop_or_zero_rate_target by all if it is not necessary.

I think it is not a problem when it is with the correct sign, and it might be a problem when the sign of the WQTotal switches, which is easy for the wells without a zero rate target.

So far we do not know how to handle wells with the opposite production/injection direction.

From the viewpoint of the code, we probably want to avoid different functions to be used for the same purpose, otherwise, we might get into trouble in debugging.
And also, potentially, the function stoppedOrZeroRateTargetIndividual can be removed if we use stopppedOrZeroRateTarget here, which is another benefit.

I think it is not a problem when it is with the correct sign, and it might be a problem when the sign of the WQTotal switches, which is easy for the wells without a zero rate target.

I beleive WQTotal is not allowed to switch sign in the Newton update, so it either has the right sign or is zero.

steink · 2024-03-19T12:07:18Z

opm/simulators/wells/MultisegmentWell_impl.hpp

@@ -684,7 +684,7 @@ namespace Opm

        const double dFLimit = this->param_.dwell_fraction_max_;
        const double max_pressure_change = this->param_.max_pressure_change_ms_wells_;
-        const bool stop_or_zero_rate_target = this->stopppedOrZeroRateTarget(summary_state, well_state);
+        const bool stop_or_zero_rate_target = this->stoppedOrZeroRateTargetIndividual(summary_state, well_state);


Same comment as above.

steink · 2024-03-19T12:08:01Z

opm/simulators/wells/StandardWell_impl.hpp

@@ -692,7 +694,7 @@ namespace Opm
    {
        if (!this->isOperableAndSolvable() && !this->wellIsStopped()) return;

-        const bool stop_or_zero_rate_target = this->stopppedOrZeroRateTarget(summary_state, well_state);
+        const bool stop_or_zero_rate_target = this->stoppedOrZeroRateTargetIndividual(summary_state, well_state);


Same comment as for MS-wells

steink · 2024-03-19T12:17:15Z

opm/simulators/wells/StandardWell_impl.hpp

@@ -1183,7 +1185,7 @@ namespace Opm
        constexpr double stopped_factor = 1.e-4;
        // use stricter tolerance for dynamic thp to ameliorate network convergence
        constexpr double dynamic_thp_factor = 1.e-1;
-        if (this->stopppedOrZeroRateTarget(summary_state, well_state)) {
+        if (this->stoppedOrZeroRateTargetIndividual(summary_state, well_state)) {
            tol_wells = tol_wells*stopped_factor;


I think it's a bit strange that zero target wells should be solved with a much stricter tollerance than others. For convenience, I've also skipped checking for zero targets from groups here.

There might be different approaches to address this, but it improves the results for some problematic cases.

#4572
#4556

Possibly we should use stricter tolerance by all.

If we can manage tighten the well tolerance by all, then this part of the logic can be removed. (#5280). You might have had some suggestions regarding the tighter well tolerance, we can discuss.

It was a temporary fix for a reported issue. And it is good to remove this extra logic. In the well code, there are many ad-hoc logic, and , and this one is not among the complicated ones. But it is always good to reduce them with time going forward.

Thanks, would be happy to discuss stricter well-tolerances overall. Especially for standard wells I think this is a good idea!

steink · 2024-03-19T12:38:03Z

I've restructered everything here, and renamed the original functions wellUnderZeroRateTarget and stoppedOrZeroRateTarget to wellUnderZeroRateTargetIndividual and stoppedOrZeroRateTargetIndividual, respectively. The functions wellUnderZeroRateTarget and stoppedOrZeroRateTarget now also include checking for zero targets from groups. I've updated the usage of these functions where I see it necessary (like forming of control equations and in the iterateWellEqWithSwitching-versions).
Since the checking for zero targets from groups is a bit more involved, I've skipped this where I don't think it is important (see comments). Hence, these places now call the *Individual versions and the behaviour is unchanged from master.

GitPaean · 2024-03-21T10:29:01Z

jenkins build this please

GitPaean · 2024-03-21T11:55:44Z

I've restructered everything here, and renamed the original functions wellUnderZeroRateTarget and stoppedOrZeroRateTarget to wellUnderZeroRateTargetIndividual and stoppedOrZeroRateTargetIndividual, respectively. The functions wellUnderZeroRateTarget and stoppedOrZeroRateTarget now also include checking for zero targets from groups. I've updated the usage of these functions where I see it necessary (like forming of control equations and in the iterateWellEqWithSwitching-versions). Since the checking for zero targets from groups is a bit more involved, I've skipped this where I don't think it is important (see comments). Hence, these places now call the *Individual versions and the behaviour is unchanged from master.

Thanks for the update. Will need to think through and get back with suggestions as a whole.

GitPaean · 2024-03-21T11:57:48Z

At the same time, can you check the Jenkins failures to see whether the failures are relevant? Especially the ones with related to MOD4_GRP.

steink · 2024-03-21T12:46:45Z

At the same time, can you check the Jenkins failures to see whether the failures are relevant? Especially the ones with related to MOD4_GRP.

Yes, will look into what's going on with these.

steink · 2024-04-02T09:29:04Z

Have looked at the four failing tests here. NETWORK-01-REROUTE_ and NETWORK-01-REROUTE_STD match very closely. For MOD4_GRP_GEFAC there is slightly different time-stepping with the PR, but otherwise close match.

Looked a bit more closely into MOD4_GRP. This is a very problematic test where both MASTER and PR struggles a lot towards the end with control switching oscillations resulting in extremely short time steps. PR struggles a bit more than MASTER here, but since both show very erratic behavour, I can'r really faviour one above the other. Besides this last problematic part, PR and MASTER match closely.

It's probably worthwhile to look more closely into why MOD4_GRP is so problematic, and try to figure out whether the problem is with flow or with the test itself. Currently, it's not well suited as a test (due to the problems, it's also quite computationally expensive).

GitPaean · 2024-04-11T20:37:52Z

jenkins build this please

GitPaean

Sorry for the delay. I left some comments regarding how to organize the code. Let me know if you need some help or want some discussion.

I do realize this PR is a incremental change that fixes things without breaking other things, while still trying to make the code usage more systematical or reduce the number of the functions whenever possible.

Let me know.

opm/simulators/wells/WellInterface_impl.hpp

GitPaean · 2024-04-11T21:36:39Z

opm/simulators/wells/WellInterface_impl.hpp

+            return this->wellUnderZeroRateTargetIndividual(summaryState, well_state);
+        } else {
+            const auto& group_state = simulator.problem().wellModel().groupState();
+            return wellUnderZeroRateTargetGroup(simulator, well_state, group_state, deferred_logger);


by getting the Schedule and SummaryState from simulator, the function wellUnderZeroRateTargetGroup() can be moved to WellInterfaceGeneric.

This was was my first intention, but was not able to do it without a lot of other code-changes. If you see some smarter way to move it there, I'll be happy to.

GitPaean · 2024-04-11T21:43:06Z

opm/simulators/wells/MultisegmentWell_impl.hpp

@@ -159,7 +159,7 @@ namespace Opm
                           const WellState& well_state,
                           DeferredLogger& /* deferred_logger */)
    {
-        const bool stop_or_zero_rate_target = this->stopppedOrZeroRateTarget(summary_state, well_state);
+        const bool stop_or_zero_rate_target = this->stoppedOrZeroRateTargetIndividual(summary_state, well_state);


From the viewpoint of the code, we probably want to avoid different functions to be used for the same purpose, otherwise, we might get into trouble in debugging.
And also, potentially, the function stoppedOrZeroRateTargetIndividual can be removed if we use stopppedOrZeroRateTarget here, which is another benefit.

GitPaean · 2024-04-11T21:59:48Z

opm/simulators/wells/StandardWell_impl.hpp

@@ -1183,7 +1185,7 @@ namespace Opm
        constexpr double stopped_factor = 1.e-4;
        // use stricter tolerance for dynamic thp to ameliorate network convergence
        constexpr double dynamic_thp_factor = 1.e-1;
-        if (this->stopppedOrZeroRateTarget(summary_state, well_state)) {
+        if (this->stoppedOrZeroRateTargetIndividual(summary_state, well_state)) {
            tol_wells = tol_wells*stopped_factor;


If we can manage tighten the well tolerance by all, then this part of the logic can be removed. (#5280). You might have had some suggestions regarding the tighter well tolerance, we can discuss.

It was a temporary fix for a reported issue. And it is good to remove this extra logic. In the well code, there are many ad-hoc logic, and , and this one is not among the complicated ones. But it is always good to reduce them with time going forward.

moving the function wellUnderZeroRateTargetGroup to WellInterfaceFluidSystem removing the function stoppedOrZeroRateTargetIndividual to avoid extra logic

steink · 2024-04-23T07:55:35Z

jenkins build this please

GitPaean · 2024-04-23T08:20:02Z

jenkins build this please

atgeirr

Looks good and can be merged assuming the test failures have been checked.

For later: Will look at improving some interfaces to possibly reduce the need to pass const Simulator& around, and maybe avoid using logging and exceptions in the TargetCalculator.

GitPaean · 2024-04-23T10:49:40Z

The testing failures need to be double checked.

GitPaean · 2024-04-23T10:57:38Z

Not remembering what happened with the regression test before the refactoring, will create a PR for testing purpose.

steink · 2024-04-23T13:28:31Z

I've gone through the failures and think they look OK. For most of them it's just a result of different time-stepping/different number of time-steps which in many cases leads to quite different results since time-steps are so large relative to the dynamics.
Did not check PYACTION_WCONPROD.

Case	Status	Comments
NETWORK-01	OK
NETWORK-01_STANDARD	OK
NETWORK-01-REROUTE	OK
NETWORK-01-REROUTE_STD	OK
UDQ_WCONPROD	check	OK, different time-stepping
UDQ_ACTIONX	check	OK, different time-stepping
PYACTION_WCONPROD	check
9_3A_GINJ_REIN-G_MSW	tstep	OK, different time-stepping
9_3B_GINJ_GAS_EXPORT_MSW	tstep	OK, different time-stepping
9_3C_GINJ_GAS_GCONSUMP_MSW	check	OK, different time-stepping
9_3E_GAS_MIN_EXPORT_STW	OK
9_3D_GINJ_GAS_MAX_EXPORT_MSW	check	OK, different time-stepping
9_4A_WINJ_MAXWRATES_MAXBHP_GCONPROD_1L_STW	tstep	Quite different results/time-stepping, time-steps much too long to catch dynamics accurately
9_4A_WINJ_MAXWRATES_MAXBHP_GCONPROD_1L_MSW	tstep	Quite different results/time-stepping, time-steps much too long to catch dynamics accurately
9_4C_WINJ_GINJ_VREP-W_REIN-G_STW	check	OK, different time-stepping
9_4B_WINJ_VREP-W_MSW	tstep	OK, different time-stepping
9_4D_WINJ_GINJ_GAS_EXPORT_STW	tstep	OK, different time-stepping
9_4C_WINJ_GINJ_VREP-W_REIN-G_MSW	check	Different time-stepping, mostly close match, except master violates group oil rate and pr violates group gas rate
9_4D_WINJ_GINJ_GAS_EXPORT_MSW	check	Different time-stepping, mostly close match, except master violates group oil rate
MOD4_UDQ_ACTIONX	tstep	OK, slightly different time-stepping
ACTIONX_GCONPROD	rsm?	Not sure why this triggered failure, results are almost identical
MOD4_GRP	tstep	OK, different time-stepping,problematic case, but pr less problematic
MOD4_GRP_GEFAC	tstep	OK, different time-stepping
KRNUM-03X	check	OK, different time-stepping (very long time-steps vs dynamics)
KRNUM-03Y	check	OK, different time-stepping (very long time-steps vs dynamics)

GitPaean · 2024-04-24T11:01:10Z

The following is some notes for information.

The thing is a little worrying is that, when we check the group target for a well, the group target might not be ready.

I looked at the case KRNUM-03X,

the SCHUEDULE setup for the well F-1H is as follows, with the current code, at the beginning, it will think F-1H is under zero injection group target, probably because the production rate is not ready yet so VREP get 0.

WELSPECS
 'F-1H'  'S1'   3    3      1*   GAS     1*   1*   SHUT 1* 1* 1* /
/

GCONINJE
 'S1'   'GAS'	 'VREP'  3*	 1.020    'NO'  5* /
/

WCONINJE
-- Well_name    Type    Status  Ctrl    SRate1  Rrate   BHP     THP     VFP
  'F-1H'        GAS   OPEN    GRUP    8.0E6    1*      300.0	1*	1*     /
/

The VREP group control with above similar setup should be causes for many regression failures. Because ideally, for many cases, there should be no wells under zero rate target at all and should no be affected.

steink · 2024-04-24T11:13:57Z

The thing is a little worrying is that, when we check the group target for a well, the group target might not be ready.

Yes, but don't you think it's better in this case to treat it properly as a zero rate well than trying to solve it with an individual zero rate (as in master) which even might give singular system?

GitPaean · 2024-04-24T11:23:45Z

Yes, but don't you think it's better in this case to treat it properly as a zero rate well than trying to solve it with an individual zero rate (as in master) which even might give singular system?

It took some thinking to understand your point. Basically, you meant that without the checking, we will solve it with zero rate target in the master branch. I think your point is valid and I agree.

My point was that stopppedOrZeroRateTarget() can be improved to reflect better. Or we should have different ways in handling the VREP injections, but that should be outside of the scope of this PR.

steink · 2024-04-24T11:41:07Z

It took some thinking to understand your point. Basically, you meant that without the checking, we will solve it with zero rate target in the master branch. I think your point is valid and I agree.

Sorry, was trying to finnish the sentence before a meeting ;)

My point was that stopppedOrZeroRateTarget() can be improved to reflect better. Or we should have different ways in handling the VREP injections, but that should be outside of the scope of this PR.

I see you point, I agree.

GitPaean · 2024-04-24T13:20:23Z

I checked the regression test with PYACTION_WCONPROD. It looks like the different results are also from different time-stepping.

If we regulate the time step with --solver-max-time-step-in-days=30, the results will be identical between this PR and master branch.

GitPaean · 2024-04-24T15:02:13Z

For the test failure, ACTIONX_GCONPROD, the message is not very clear.

Loading rft file /var/lib/jenkins/workspace/opm-simulators-PR-builder/deps/opm-tests/actionx/opm-simulation-reference/flow/ACTIONX_GCONPROD.RFT  .... done
Loading rft file /var/lib/jenkins/workspace/opm-simulators-PR-builder/mpi/build-opm-simulators/tests/results/flow+actionx_gconprod/ACTIONX_GCONPROD.RFT  .... done

Well: OP01 date: 2017/12/31
Comparing: TIME ...  done.
Comparing: DATE ...  done.
Comparing: WELLETC ...  done.
Comparing: CONIPOS ...  done.
Comparing: CONJPOS ...  done.
Comparing: CONKPOS ...  done.
Comparing: HOSTGRID ...  done.
Comparing: DEPTH ...  done.
Comparing: PRESSURE ...  done.
Comparing: SWAT ...  done.
Comparing: SGAS ...  done.

Program threw an exception: [/var/lib/jenkins/workspace/opm-simulators-PR-builder/deps/opm-common/test_util/compareECL.cpp:265] 1 errors encountered in comparisons.

GitPaean · 2024-04-24T15:19:01Z

With some discussion and help from @bska , the regression failure for ACTIONX_GCONPROD is related to RUNSUM keyword.

Since the simulation results look generally fine and for this failure, we can update reference. But we need to wait for the release processing to unfreeze the reference result update.

steink requested review from atgeirr and GitPaean February 28, 2024 15:17

GitPaean reviewed Mar 7, 2024

View reviewed changes

opm/simulators/wells/WellInterface_impl.hpp Outdated Show resolved Hide resolved

GitPaean reviewed Mar 7, 2024

View reviewed changes

opm/simulators/wells/WellInterface_impl.hpp Outdated Show resolved Hide resolved

GitPaean reviewed Mar 7, 2024

View reviewed changes

opm/simulators/wells/WellInterface_impl.hpp Outdated Show resolved Hide resolved

steink force-pushed the check_zero_target_from_group branch from d3abaad to 2355c4c Compare March 19, 2024 11:52

steink commented Mar 19, 2024

View reviewed changes

steink changed the title ~~Draft for discussion: when checking for zero target rates, also check wells under group control~~ When checking for zero target rates, also check wells under group control where required Mar 19, 2024

steink marked this pull request as ready for review March 19, 2024 12:38

GitPaean requested changes Apr 11, 2024

View reviewed changes

steink and others added 5 commits April 22, 2024 15:33

include group controls for zero rate target checking

018d40c

Include checking for zero targets from groups where required

3c3ea0c

refactoring lightly the PR opm-simulators#5232

e33a068

moving the function wellUnderZeroRateTargetGroup to WellInterfaceFluidSystem removing the function stoppedOrZeroRateTargetIndividual to avoid extra logic

fixing the mistakes introduced by rebasing

4cd5792

remvoing two virtuals

852678b

steink force-pushed the check_zero_target_from_group branch from 2355c4c to 852678b Compare April 23, 2024 07:54

GitPaean approved these changes Apr 23, 2024

View reviewed changes

atgeirr approved these changes Apr 23, 2024

View reviewed changes

GitPaean mentioned this pull request Apr 23, 2024

[testing PR] check_zero_target_from_group #5300

Closed

When checking for zero target rates, also check wells under group control where required #5232

Are you sure you want to change the base?

When checking for zero target rates, also check wells under group control where required #5232

Conversation

steink commented Feb 28, 2024

GitPaean commented Feb 29, 2024

GitPaean commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steink commented Mar 19, 2024

GitPaean commented Mar 21, 2024

GitPaean commented Mar 21, 2024

GitPaean commented Mar 21, 2024

steink commented Mar 21, 2024

steink commented Apr 2, 2024

GitPaean commented Apr 11, 2024

GitPaean left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steink commented Apr 23, 2024

GitPaean commented Apr 23, 2024

atgeirr left a comment

Choose a reason for hiding this comment

GitPaean commented Apr 23, 2024

GitPaean commented Apr 23, 2024

steink commented Apr 23, 2024 • edited by bska

GitPaean commented Apr 24, 2024

steink commented Apr 24, 2024

GitPaean commented Apr 24, 2024

steink commented Apr 24, 2024

GitPaean commented Apr 24, 2024

GitPaean commented Apr 24, 2024

GitPaean commented Apr 24, 2024

steink commented Apr 23, 2024 •

edited by bska