Add support for WTHPH+WBHPH keywords #1162

sveinung-r · 2017-11-29T07:40:16Z

No description provided.

joakim-hove · 2017-11-30T10:59:44Z

The code in itself looks perfectly OK, but there have been insane amounts of confusion regarding BHP in this code, and I fear the current PR will add to that. I will look carefully through - hopefully we can guard against this confusion by just using better names, but I am not certain. The problem is that the term BHP is used in (at least) three different ways:

The flowing bottom hole pressure in the well - something calculated by the simulator.
A BHP control limit imposed by the user, and used as a well boundary condition.
The historical BHP values when the well is run in historical mode - which is what this PR is about.

What I fear is that there is some non-trivial interaction between 2 and 3 above, need to verify that before this can be merged.

joakim-hove · 2017-11-30T13:30:07Z

I quite frankly do not fully understand if this is enough; but I think this should be enough:

The new member variables should be renamed BHPH and THPH for historical mode.
In the function updating production properties the historical flag must be consulted, and only if this is true the BHPH and THPH member variables should be set.

sveinung-r · 2017-11-30T13:54:05Z

I opened an issue ( #1163 ) regarding the ambiguities in WellProductionProperties and WellInjectionProperties as these are used for representing both history matching and control data, with a boolean property separating the two. Having separate types for history matching and control would enable terms such as BHP (as well as Oil production rate etc. ) to be interpreted as historic data or targets/limits from context.

As far as I understand, case 1 will not be represented in WellProductionProperties and WellInjectionProperties.

GitPaean · 2017-11-30T14:08:39Z

Do the keywords WTHPH and WBHPH only affect the summary output? The output might need to wait the WCONHIST and WCONINJH parsing to be updated / corrected first. This PR looks to me is trying to partly to correct the treatment related to WCONINJH, and the real problem here is actually the one you pointed out in #1163.

joakim-hove · 2017-11-30T14:14:45Z

Do the keywords WTHPH and WBHPH only affect the summary output?

These are not keywords, but suggested class member names. Should only affect output.

sveinung-r · 2017-11-30T14:28:04Z

To clarify, WTHPH and WBHPH are keywords for the summary output (THPH and BHPH are suggested class member names). This PR is adding support for historic BHP and THP data to be accessed in opm-output in order to support these keywords, as these are not read from WCONHIST and WCONINJH as of now.

GitPaean · 2017-11-30T14:30:44Z

I have not looked at WCONINJH, but for WCONHIST, the observed values (at least the rates) are read as limits. Correcting this will impact the simulator running. I will try to provide a solution.

The simulator manages to work with other further processing, so some adjustment needs to be done there.

sveinung-r · 2017-11-30T14:32:05Z

Which values were read as limits?

GitPaean · 2017-11-30T14:35:23Z

Which values were read as limits?

If I understand the function correctly,

WellProductionProperties WellProductionProperties::history(

sveinung-r · 2017-12-01T08:00:17Z

The rates are read into the same variables for limits and historic data. However, this is not the case for BHP and THP. Since the BHPLimit variable is used for other purposes in the code this can not be done for this variable. Using BHPH and THPH could help to make this distinction somewhat clearer.

It is true that solving issue #1163 would solve this. Should I fix this PR or await an alternative solution?

GitPaean · 2017-12-01T08:54:10Z

The rates are read into the same variables for limits and historic data.

Do you mean in the keyword like WCONHIST, the rates not under control serve also as limits not just historical data? I thought they will just be observed data for comparison while will serve as control/limit.

GitPaean · 2017-12-01T09:29:10Z

A question about SUMMARY keyword, do we support customization the output of SUMMARY, or what we output through summary is fixed by the simulator.

The definition of the keyword SUMMARY is rather simple without much content.

The support of the keyword WTHPH+WBHPH should be defined in the SUMMARY first? or we will always output them, then it is a little messy for non-history matching wells.

GitPaean · 2017-12-01T09:41:25Z

It is true that solving issue #1163 would solve this. Should I fix this PR or await an alternative solution?

Fixing #1163 will involve much more than this PR's purpose (for output), maybe you can wait a little bit. Hopefully, we will be able to provide a fix soon.

joakim-hove · 2017-12-01T09:48:28Z

A question about SUMMARY keyword, do we support customization the output of SUMMARY, or what we output through summary is fixed by the simulator.

Yes - the parser checks the SUMMARY section and creates a SummaryConfig object with all the requested summary output. What we do not check/validate very well is if the summary output requested is consistent with the abilities of the current simulation; i.e. if you ask for oil rate in a gas+water system you will get no error/warnings.

The support of the keyword WTHPH+WBHPH should be defined in the SUMMARY first?

yes - and that is already implemented and works.

or we will always output them, then it is a little messy for non-history matching wells.

We will only output them if asked for - but we will output them also for non-history matching wells if they are requested. I guess you could call that a logical f...up.

GitPaean · 2017-12-01T09:53:53Z

yes - and that is already implemented and works.

good, then we will focus on make the correct values in place

joakim-hove · 2017-12-01T09:54:59Z

Fixing #1163 will involve much more than this PR's purpose (for output), maybe you can wait a little bit. Hopefully, we will be able to provide a fix soon.

The plan is to merge this PR (#1162) quite soon, that should be a pure additive change not conflicting with anything[1].

I think I agree that a Good Solution™ involves splitting the well properties objects into a historical class and a prediction class - along the lines of #1032, but experience has shown that these topics (historical/prediction/rates/controls/ ...) is a massive conceptual clusterf..k in Eclipse - so getting a correct and easily maintainable solution of this in our code might be quite non trivial.

[1]: Please scream out if you disagree strongly.

GitPaean · 2017-12-01T10:00:40Z

The plan is to merge this PR (#1162) quite soon, that should be a pure additive change not conflicting with anything.

Sure. Please feel free to do that. My focus is making the treatment of history matching wells correct. Especially, when we remove the usage of the WellControls from opm-core, using the injection and production properties from opm-parser directly, it will look bad if we redo the same trick with WellControls again with the injection and production properties from opm-parser.

joakim-hove · 2017-12-01T11:52:28Z

lib/eclipse/tests/ScheduleTests.cpp

+        " 'P' 'OP' 9 9 1* 'OIL' 1* / \n"
+        " 'I' 'OP' 9 9 1* 'WATER' 1* / \n"
+        "/\n"
+        "WCONHIST\n"


Add a WCONPROD keyword with well P2- set the BHP and THP items of that well, and verify that the BHPH and THPH variables have not been set.

joakim-hove · 2017-12-01T11:55:24Z

lib/eclipse/EclipseState/Schedule/WellProductionProperties.cpp

@@ -86,6 +86,9 @@ namespace Opm {
                throw std::invalid_argument("Setting CMODE to unspecified control");
        }

+        p.BHPH = record.getItem("BHP").getSIDouble(0);


The BHP and THP items do not have default values, so this must be "protected" with hasValue(0) checks.

joakim-hove · 2017-12-01T11:56:20Z

lib/eclipse/EclipseState/Schedule/Schedule.cpp

@@ -740,6 +740,13 @@ namespace Opm {
            }
            properties.predictionMode = false;

+            properties.BHPH = record.getItem( "BHP" ).hasValue(0)


The BHPH and THPH elements should be initialized to zero; no need to explicitly assign to 0.0.

joakim-hove

A couple of small comments, then this can be merged.

joakim-hove · 2017-12-01T12:54:08Z

jenkins build this with downstreams opm-output=222 please

sveinung-r force-pushed the Well_pri_3_keywords branch 2 times, most recently from 6d33086 to c58cd09 Compare December 1, 2017 10:57

sveinung-r mentioned this pull request Dec 1, 2017

Add WTHPH+WBHPH keywords OPM/opm-output#222

Merged

joakim-hove reviewed Dec 1, 2017

View reviewed changes

joakim-hove approved these changes Dec 1, 2017

View reviewed changes

sveinung-r force-pushed the Well_pri_3_keywords branch from c58cd09 to f8bbb7c Compare December 1, 2017 12:18

Add support for WTHPH+WBHPH keywords

f78373a

sveinung-r force-pushed the Well_pri_3_keywords branch from f8bbb7c to f78373a Compare December 1, 2017 12:20

joakim-hove merged commit e95ec55 into OPM:master Dec 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for WTHPH+WBHPH keywords #1162

Add support for WTHPH+WBHPH keywords #1162

sveinung-r commented Nov 29, 2017

joakim-hove commented Nov 30, 2017

joakim-hove commented Nov 30, 2017

sveinung-r commented Nov 30, 2017

GitPaean commented Nov 30, 2017 •

edited

Loading

joakim-hove commented Nov 30, 2017

sveinung-r commented Nov 30, 2017

GitPaean commented Nov 30, 2017 •

edited

Loading

sveinung-r commented Nov 30, 2017

GitPaean commented Nov 30, 2017

sveinung-r commented Dec 1, 2017

GitPaean commented Dec 1, 2017

GitPaean commented Dec 1, 2017

GitPaean commented Dec 1, 2017

joakim-hove commented Dec 1, 2017

GitPaean commented Dec 1, 2017

joakim-hove commented Dec 1, 2017 •

edited

Loading

GitPaean commented Dec 1, 2017

joakim-hove Dec 1, 2017

joakim-hove Dec 1, 2017

joakim-hove Dec 1, 2017

joakim-hove left a comment

joakim-hove commented Dec 1, 2017

Add support for WTHPH+WBHPH keywords #1162

Add support for WTHPH+WBHPH keywords #1162

Conversation

sveinung-r commented Nov 29, 2017

joakim-hove commented Nov 30, 2017

joakim-hove commented Nov 30, 2017

sveinung-r commented Nov 30, 2017

GitPaean commented Nov 30, 2017 • edited Loading

joakim-hove commented Nov 30, 2017

sveinung-r commented Nov 30, 2017

GitPaean commented Nov 30, 2017 • edited Loading

sveinung-r commented Nov 30, 2017

GitPaean commented Nov 30, 2017

sveinung-r commented Dec 1, 2017

GitPaean commented Dec 1, 2017

GitPaean commented Dec 1, 2017

GitPaean commented Dec 1, 2017

joakim-hove commented Dec 1, 2017

GitPaean commented Dec 1, 2017

joakim-hove commented Dec 1, 2017 • edited Loading

GitPaean commented Dec 1, 2017

joakim-hove Dec 1, 2017

Choose a reason for hiding this comment

joakim-hove Dec 1, 2017

Choose a reason for hiding this comment

joakim-hove Dec 1, 2017

Choose a reason for hiding this comment

joakim-hove left a comment

Choose a reason for hiding this comment

joakim-hove commented Dec 1, 2017

GitPaean commented Nov 30, 2017 •

edited

Loading

GitPaean commented Nov 30, 2017 •

edited

Loading

joakim-hove commented Dec 1, 2017 •

edited

Loading