Fully internalized the JFUNC keyword #970

pgdr · 2016-11-09T14:31:33Z

Fully internalized the JFUNC keyword into class JFuncTable.hpp
Added JFUNC flag, getJFuncColumn() and getJFuncTable() in TableManager
added protected member m_jfunc to SimpleTable
Added getJFuncColumn to accompany getPcowColumn
Throws if pressure or jfunc is accessed inappropriately
- ... meaning if getPcowColumn is called when JFUNC is in deck, or
- getJFuncColumn is called when JFUNC is not in deck
added tests for throwing and for getJFuncColumn
In the event that one tries to get "PCOW" or "PCOG" via getColumn
- ... this will throw if JFUNC is present in the deck.
Added tests.

pgdr · 2016-11-09T14:33:37Z

See #956 for prior discussions around the JFUNC keyword.

joakim-hove · 2016-11-10T07:46:21Z

opm/parser/eclipse/EclipseState/Tables/JFuncTable.hpp

+{
+public:
+
+    enum Flag {


We should start using the "new" enum class consistently: http://www.cprogramming.com/c++11/c++11-nullptr-strongly-typed-enum-class.html

joakim-hove · 2016-11-10T07:52:52Z

opm/parser/eclipse/EclipseState/Tables/JFuncTable.hpp

+
+    inline void init(const DeckKeyword& kw) {
+        const auto& rec = kw.getRecord(0);
+        if (rec.hasItem("FLAG")) {


As long as the DeckKeyword instance indeed corresponds to JFUNC keyword this test will always return true; and if it returns false that is not because a default has been applied - but rather because the wrong keyword has been passed in - which is throw material.

joakim-hove · 2016-11-10T07:53:49Z

opm/parser/eclipse/EclipseState/Tables/JFuncTable.hpp

+        XY, X, Y, Z
+    };
+
+    inline void init(const DeckKeyword& kw) {


This could just be constructor - no need to go through a separate init function?

Could maybe implement operator bool( ) in the Jfunc class, then the instance could be constructed as part of the TableManager initialization list, and the Jfunc member instance could just evaluate to false if the Deck does not have the JFUNC keyword.

joakim-hove · 2016-11-10T07:54:44Z

opm/parser/eclipse/EclipseState/Tables/JFuncTable.hpp

+
+class DeckKeyword;
+
+class JFuncTable


This is not really a table - just the JFunc keyword - which will be used when interpreting tables.

joakim-hove · 2016-11-10T07:59:25Z

opm/parser/eclipse/EclipseState/Tables/JFuncTable.hpp

+        if (flag != GAS)
+            owSurfaceTension = rec.getItem("OW_SURFACE_TENSION").get<double>(0);
+
+        if (rec.hasItem("ALPHA_FACTOR"))


Same comment about hasItem - it is in my opinion not necessary, but if you want to check for default behavior it is the item which should be checked:

const auto item = rec.getItem("ALPHA_FACTOR"): // Check if the ALPHA_FACTOR has a value - as long a default is specified in the Json this will *always* return true: if ( item.hasValue(0)) alphaFactor = item.get<double>( 0 ); // Check a value has been set explicitly: if (!item.defaultApplied(0)) alphaFactor = item.get<double>( 0 );

joakim-hove · 2016-11-10T08:03:37Z

opm/parser/eclipse/EclipseState/Tables/JFuncTable.hpp

+    }
+
+private:
+    Flag      flag = BOTH;             // JFUNC flag: WATER, GAS, or BOTH.  Default BOTH


I would prefer if these default values were removed - they are already specified in the Json configuration, and if the values are requested unconditionally from the deck we will correctly get either the default or the value entered explicitly.

pgdr · 2016-11-10T09:31:20Z

Thanks for the review, Joakim. I've fixed all of the items, except the one where you request deleting the default values.

Hold off merging anyway until we synchronize efforts with Andreas.

pgdr · 2016-11-10T09:43:31Z

Thanks for the review, Joakim. I've fixed all of the items.

jokva · 2016-11-10T10:03:15Z

A quick note about commit --fixup:

If you have multiple fixups all destined to be squashed into one commit you need to fixup into the same base commit, not the children, for autosquash to pick up on them.

pgdr · 2016-11-10T10:33:59Z

Yeah, I just let emacs do the job. :)

jokva · 2016-11-10T11:25:47Z

opm/parser/eclipse/EclipseState/Tables/JFunc.cpp

+        else if (kw_flag == "WATER")
+            flag = Flag::WATER;
+        else if (kw_flag == "GAS")
+            flag = Flag::GAS;


Is flag mandatory, or is it optional (defaulted to BOTH maybe)? If the former you need to set it unconditionally, ideally through its declaration.

in JFUNC, flag is optional and defaulted to BOTH. Meaning that if it is not specified in deck, through the JFUNC json, it will be given as BOTH.

There is a potential issue here, namely if a user specifies JFUNC GARBAGE/ in the deck. Then we might reach a state where we read an uninitialized flag.

I could add a final else throw unrecognized flag GARBAGE if you'd like.

I think that's a good idea yeah, otherwise we potentially reach a much more subtle wrong state.

I'm now throwing on garbage FLAG or DIRECTION.

I guess throwing on invalid FLAG / DIRECTION is a good idea.

pgdr · 2016-11-17T11:30:18Z

@andlaus ping

andlaus · 2016-11-17T12:11:30Z

pong: I had no time for this this week. (have been busy with making flow_ebos merge-ready.)

pgdr · 2016-11-24T09:02:44Z

@andlaus ping

andlaus · 2016-11-24T10:44:50Z

pong. on it: probably next week. (if you want to speed things up, you may also do the opm-material part.)

pgdr · 2016-11-28T12:59:27Z

@andlaus ping

(This will probably be merged pretty soon.)

andlaus · 2016-11-28T13:21:08Z

(This will probably be merged pretty soon.)

if flow_ebos (and flow_legacy) continue to work for Model 2.2 after the merge, I don't have any objections.

andlaus · 2016-12-07T11:07:14Z

@pgdr: could you please split the JFUNC-keyword internalization part into a separate PR where the tables are left as they are? I think that the JFUNC internalization is a good thing to have, whereas I'm not so sure about the tables.

pgdr · 2016-12-07T18:11:07Z

Why are you not so sure about the tables?

andlaus · 2016-12-07T19:41:45Z

I'm not sure on whether the table stuff is worth it because adapting the opm-material side to the table semantics is quite a bit of non-obvious work (as you've already discovered yourself?), most likely it does not make the resulting code easier to understand and the result may be even a bit slower than the current hack. Finally, it makes the semantics on the opm-parser side more complicated. (a.k.a. "when am I allowed to call getPcwoColumn() again?")

anyway, the JFUNC internalization is uncontroversial, so let's do this step-by-step.

pgdr · 2016-12-27T12:14:22Z

Third time's the charm.

We do now not throw on incorrect reading (or lack) of JFUNC column. This will be reversed later when opm-material is updated.

* Fully internalized JFUNC * Added JFunc.cpp, throw on wrong FLAG/DIR in JFUNC kw * added tests for JFUNC in TableManagerTests * added protected member m_jfunc to SimpleTable * Added getJFuncColumn to accompany getPcowColumn * Throws if pressure or jfunc is accessed inappropriately * ... meaning if getPcowColumn is called when JFUNC is in deck, or * getJFuncColumn is called when JFUNC is not in deck * added tests for throwing and for getJFuncColumn * SimpleTable.getColumn("PCOW/PCOG") throws if JFUNC * In the event that one tries to get "PCOW" or "PCOG" via getColumn * ... this will throw if JFUNC is present in the deck. * Added tests.

* no throw on JFUNC abuse, std::cerr * removed 'get' from function names

pgdr · 2016-12-28T18:22:06Z

@andlaus @jokva @joakim-hove I'm merging this tomorrow if I don't hear anything from you.

jokva · 2016-12-28T18:28:11Z

Go for it.

andlaus · 2016-12-28T23:50:17Z

go ahead. I'll convert opm-material to the internalized version of the keyword shortly thereafter.

pgdr changed the title ~~Fully internalized the JFUNC keyword~~ WIP Fully internalized the JFUNC keyword Nov 9, 2016

pgdr changed the title ~~WIP Fully internalized the JFUNC keyword~~ Fully internalized the JFUNC keyword Nov 9, 2016

joakim-hove reviewed Nov 10, 2016

View reviewed changes

joakim-hove suggested changes Nov 10, 2016

View reviewed changes

joakim-hove approved these changes Nov 10, 2016

View reviewed changes

jokva reviewed Nov 10, 2016

View reviewed changes

pgdr mentioned this pull request Nov 28, 2016

Use the JFUNC implementation in opm-parser OPM/opm-material#182

Closed

Pål Grønås Drange added 2 commits December 28, 2016 19:20

Revert throw, write warning to stderr

7f3137b

* no throw on JFUNC abuse, std::cerr * removed 'get' from function names

pgdr merged commit 69c26c7 into OPM:master Dec 29, 2016

pgdr deleted the jfunc-pt2 branch December 29, 2016 09:10

bska mentioned this pull request Nov 16, 2022

Noisy warning OPM/opm-common#3223

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully internalized the JFUNC keyword #970

Fully internalized the JFUNC keyword #970

pgdr commented Nov 9, 2016

pgdr commented Nov 9, 2016 •

edited

Loading

joakim-hove Nov 10, 2016

joakim-hove Nov 10, 2016

joakim-hove Nov 10, 2016

joakim-hove Nov 10, 2016

joakim-hove Nov 10, 2016

joakim-hove Nov 10, 2016

joakim-hove Nov 10, 2016

pgdr commented Nov 10, 2016

pgdr commented Nov 10, 2016

jokva commented Nov 10, 2016

pgdr commented Nov 10, 2016

jokva Nov 10, 2016

pgdr Nov 10, 2016

jokva Nov 10, 2016

pgdr Nov 10, 2016

joakim-hove Nov 10, 2016

pgdr commented Nov 17, 2016

andlaus commented Nov 17, 2016

pgdr commented Nov 24, 2016

andlaus commented Nov 24, 2016

pgdr commented Nov 28, 2016

andlaus commented Nov 28, 2016

andlaus commented Dec 7, 2016

pgdr commented Dec 7, 2016

andlaus commented Dec 7, 2016

pgdr commented Dec 27, 2016

pgdr commented Dec 28, 2016

jokva commented Dec 28, 2016

andlaus commented Dec 28, 2016


		class DeckKeyword;

		class JFuncTable

Fully internalized the JFUNC keyword #970

Fully internalized the JFUNC keyword #970

Conversation

pgdr commented Nov 9, 2016

pgdr commented Nov 9, 2016 • edited Loading

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

pgdr commented Nov 10, 2016

pgdr commented Nov 10, 2016

jokva commented Nov 10, 2016

pgdr commented Nov 10, 2016

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

pgdr commented Nov 17, 2016

andlaus commented Nov 17, 2016

pgdr commented Nov 24, 2016

andlaus commented Nov 24, 2016

pgdr commented Nov 28, 2016

andlaus commented Nov 28, 2016

andlaus commented Dec 7, 2016

pgdr commented Dec 7, 2016

andlaus commented Dec 7, 2016

pgdr commented Dec 27, 2016

pgdr commented Dec 28, 2016

jokva commented Dec 28, 2016

andlaus commented Dec 28, 2016

pgdr commented Nov 9, 2016 •

edited

Loading