OPM / opm-parser

http://www.opm-project.org
11 stars 44 forks source link

Support multiply porv #1196

Closed lars-petter-hauge closed 6 years ago

lars-petter-hauge commented 6 years ago

Support MULTIPLY functionality for PORV

Setting the "MULTIPLY" keyword to act on "PORV" would throw an exception:

"Program threw an exception: Fatal error processing MULTIPLY keyword. Tried to scale not defined keyword PORV"

Now the initPORV is called if the MULTIPLY keyword is parsed with PORV. Note that MULTIPLY for PORV will also work in the GRID section (which it does not in eclipse), but will fail if it is placed prior to setting a variable PORV depends on.

joakim-hove commented 6 years ago

jenkins build this with downstreams please

joakim-hove commented 6 years ago

There are some problems with this which need to be adressed:

  1. My estimate is that whether the is_default_initiializable flag should be true/false in about 50% of the cases; hence I really think it should be set explicitly in all cases - and not defaulted in the declaration.

  2. In the current approach the getKeyword() is excplicitly protected in the xxxMULTIPLY method by adding an assertKeyword( ) call. I would prefer if this was changed the other way around - the assertKeyword() should not add a keyword which can not be default constructed. The only way to get a non-default constructable keyword should be to support this (important ...) deck construction:

PERMX
   0 1 2 /

i.e. explicit mentions of a keyword in the input deck. That is currently handled with here - and that does not invoke the assertKeyword() method - so there should be no conflict here.

  1. The handleMULTIPLYKeyword keyword currently asks if the integer og double grid properties have the keyword in question, I think they should rather ask whether the keyword in question is supported, and then the later implementation can unconditionally call assertKeyword( ) - which will eventually blow up for a non-default-constructable keyword.

  2. You have fixed the MULTIPLY function, but the exact same fix (which should in my opinion amount to using the supportsKeyword() instead of hasKeyword() method) - should be applied to the ADD , COPY and so on methods. There is currently some copy & paste of these methods, you might want to look into something slightly more elegant.

Finally - I think we will need some quite careful analysis of which fields should have default_constructable set to true and which should have false. E.g. the ACTNUM which yiu have set explicitly in this patch should betrue - i.e. all cells active.

alfbr commented 6 years ago

I can confirm that this is tested and works as intended.

joakim-hove commented 6 years ago

Ping: What is the status here?

lars-petter-hauge commented 6 years ago

I've added WIP, we have some minor things to change.

joakim-hove commented 6 years ago

jenkins build this please

joakim-hove commented 6 years ago

jenkins build this please

joakim-hove commented 6 years ago

jenkins build this please