Keyword item names differ from item names in the OPM-FLOW manual

[kriben]

In ResInsight we are using keywords from opm-common to generate schedule files with multi-segment well data. We have created an API for adding arbitrary keyword at given date. Our current approach is to take the keyword with item values as input from the end-user. The keyword configuration is given as a python dictionary mapping from item names to values, e.g { "WELNAME": "WELL_A", "I": 1, "J": 4, "SATNUM": 3}. Here we expect the item name to be "SATNUM" based on the manual, but the mapping fails since it is named "SAT_TABLE" in the code.

We assumed that the item names in the manual and the code would be the same, and they are clearly not. That was wrong assumption on our part, but it would be nice to use find the parser keyword and add items based on the "manual name".

See example from COMPDAT below. Here there is a mismatch between WELL/WELNAME, STATE/STATUS, SAT_TABLE/SATNUM, CONNECTION_TRANSMISSIBILITY_FACTOR/CONFACT and so on.

class COMPDAT:

const std::string COMPDAT::keywordName = "COMPDAT";
const std::string COMPDAT::WELL::itemName = "WELL";
const std::string COMPDAT::I::itemName = "I";
const std::string COMPDAT::J::itemName = "J";
const std::string COMPDAT::K1::itemName = "K1";
const std::string COMPDAT::K2::itemName = "K2";
const std::string COMPDAT::STATE::itemName = "STATE";
const std::string COMPDAT::STATE::defaultValue = "OPEN";
const std::string COMPDAT::SAT_TABLE::itemName = "SAT_TABLE";
const std::string COMPDAT::CONNECTION_TRANSMISSIBILITY_FACTOR::itemName = "CONNECTION_TRANSMISSIBILITY_FACTOR";
const std::string COMPDAT::DIAMETER::itemName = "DIAMETER";
const std::string COMPDAT::Kh::itemName = "Kh";
const std::string COMPDAT::SKIN::itemName = "SKIN";
const std::string COMPDAT::D_FACTOR::itemName = "D_FACTOR";
const std::string COMPDAT::DIR::itemName = "DIR";
const std::string COMPDAT::DIR::defaultValue = "Z";
const std::string COMPDAT::PR::itemName = "PR";

We also have problems with the long names when generating comment headers based on the itemName value and right-aligning the data. Here we end-up exceeding the commercial simulator max line width (132) due to SURFACE_*_FRACTION .

Would there be interest in adding a "shortName" or "manual name" to the keyword definitions? I would be willing to do the work. Continuing with the COMPDAT example:

class COMPDAT:

const std::string COMPDAT::keywordName = "COMPDAT";
// WELL -> WELNAME 
const std::string COMPDAT::WELL::itemName = "WELL";
const std::string COMPDAT::WELL::shortName = "WELNAME";
const std::string COMPDAT::I::itemName = "I";
const std::string COMPDAT::J::itemName = "J";
const std::string COMPDAT::K1::itemName = "K1";
const std::string COMPDAT::K2::itemName = "K2";
// STATE -> STATUS
const std::string COMPDAT::STATE::itemName = "STATE";
const std::string COMPDAT::STATE::shortName = "STATUS";
const std::string COMPDAT::STATE::defaultValue = "OPEN";
// SAT_TABLE -> SATNUM
const std::string COMPDAT::SAT_TABLE::itemName = "SAT_TABLE";
const std::string COMPDAT::SAT_TABLE::shortName = "SATNUM";
// CONNECTION_TRANSMISSIBILITY_FACTOR -> CONFACT
const std::string COMPDAT::CONNECTION_TRANSMISSIBILITY_FACTOR::itemName = "CONNECTION_TRANSMISSIBILITY_FACTOR";
const std::string COMPDAT::CONNECTION_TRANSMISSIBILITY_FACTOR::shortName = "CONFACT";
// DIAMETER -> DW
const std::string COMPDAT::DIAMETER::itemName = "DIAMETER";
const std::string COMPDAT::DIAMETER::shortName = "DW";
....

With this in place we can lookup item by their "short" or "manual" name.

[bska]

Would there be interest in adding a "shortName" or "manual name" to the keyword definitions?

I appreciate the problem, but I'm a little hesitant to add something in aid of downstream/out-of-project consumers that we'll need to maintain and which has no direct benefit for the OPM parser.

I would be willing to do the work.

Is there a reason you can't have a mapping of "official item names" to "short item names" in your own product, especially if you're willing to do the work?

[atgeirr]

I believe that it would be good in general to have a match between "manual names" and "code item names". However I think setting up a separate mapping/field between them is not a good idea. I would rather have them simply be the same. When creating python dictionaries etc for these things you should base your code on the JSON definition (i.e. "code item names"), not the manual. We may (piece by piece or more en mass) modify the manual and/or JSON definitions to improve match, but the JSON will always give you the correct code to interface with the parser.

[kriben]

Would there be interest in adding a "shortName" or "manual name" to the keyword definitions?

I appreciate the problem, but I'm a little hesitant to add something in aid of downstream/out-of-project consumers that we'll need to maintain and which has no direct benefit for the OPM parser.

I understand that. The better fix would be to align the "real" item name with the manual item name, but that would break existing code for everybody (so I figured that was off the table).

I would be willing to do the work.

Is there a reason you can't have a mapping of "official item names" to "short item names" in your own product, especially if you're willing to do the work?

No, we can definitely workaround this in ResInsight. Especially if we are the only ones that need this.

[bska]

Would there be interest in adding a "shortName" or "manual name" to the keyword definitions?

I appreciate the problem, but I'm a little hesitant to add something in aid of downstream/out-of-project consumers that we'll need to maintain and which has no direct benefit for the OPM parser.

I understand that. The better fix would be to align the "real" item name with the manual item name, but that would break existing code for everybody (so I figured that was off the table).

We're willing to break existing code in every release, so renaming items is not out of the question. In general, the manual should reflect what the simulator does/uses, not invent names for its own purpose.

I would be willing to do the work.

Is there a reason you can't have a mapping of "official item names" to "short item names" in your own product, especially if you're willing to do the work?

No, we can definitely workaround this in ResInsight. Especially if we are the only ones that need this.

Cool. If ResInsight implements its own solution, you could even emit a "translation table" at the beginning of each keyword, e.g., for the COMPDAT example from before:

COMPDAT
-- Item Name Aliases
-- WELNAME = WELL
-- STATUS = STATE
-- SATNUM = SAT_TABLE
-- CTF = CONNECTION_TRANSMISSIBILITY_FACTOR
-- DW = DIAMETER
--
-- WELNAME   I   J  K1  K2  STATUS  SATNUM      CTF      DW
    'P-52'  11  22  41  42  'OPEN'      17   123.45 0.54321 /
/

[magnesj]

Following up on @atgeirr's point that the long-term fix is to align the code item names with the manual, and @bska's note that renaming items is acceptable even when it breaks downstream code:

One thing that makes such renames risky today is that item names are referenced as string literals throughout the parser, e.g. record.getItem("RATE"). There are roughly 550 such literal call sites (plus ~700 that already use the templated form), so a rename currently has no compiler safety net — a missed or mistyped string only surfaces at runtime.

As a small experiment I converted a few of the Well property handlers to the type-safe templated lookup that already exists in DeckRecord, e.g. record.getItem<ParserKeywords::WCONINJE::RATE>() (with a local using Kw = ParserKeywords::WCONINJE; to keep the call sites short):

magnesj#3

With the call sites going through the generated ParserKeywords::KEYWORD::ITEM classes instead of free-form strings, renaming an item in the JSON definition becomes a compile-time-checked change rather than a manual string hunt. That would make the item-name/manual alignment described here much safer to carry out incrementally.

Main robustness improvement

If all use of a keyword is based on the template name, the string lookup can be made private and you achieve compile time check on correct keyword usage.

Opportunity

If this approach is implemented, it is not possible to look up parameter by a text string. The string representation can be updated to match the agreed upon parameter name, preferably a shorter name like the names used in the reference manual.

[akva2]

my .02c; independent of the discussion, that is a very welcome change. we should do this in two steps though, first move everything to kw form. then we can consider removing the string lookup after. if you let claude loose on the whole task, tell it to split it up a little, ie, not one huge commit but per directory or such granularity to not fill up the human reviewers context window.

[bska]

Moving away from getItem(string) and towards getItem<I>() is what we've wanted to do for a long time anyway, so that's a good idea. We'd need a little bit of refactoring in some places however, for instance in our low-level COMPDAT handling, since there are cases for which the same back-end implementation is used for multiple keywords (COMPDAT and COMPDATL in this case) but we can make exceptions where needed.

then we can consider removing the string lookup after

We can at least make it private. The template version of getItem currently just calls the string version with the itemName as an argument.

The string representation can be updated to match the agreed upon parameter name, preferably a shorter name like the names used in the reference manual.

Item name length will not be a consideration here. The parser benefits from descriptive names as it needs to solve a very different problem than what ResInsight's editor support does. As for the manual, it needs to use the same item names that the parser uses, not invent its own conventions.