Handle partially supported options manually. #762

qilicun · 2016-07-14T01:36:11Z

There are some options of the keywords only partially supported by flow, handle them automatically is not that easy, so we decided to handle them manually, the lucky thing is that the number of these kind of keywords is limited.
Any feedback and suggestions are welcome.

joakim-hove · 2016-07-14T08:24:32Z

I must say I am a bit sceptical to this - mainly because I think it will be a significant maintainance burden; and the moment we start reporting issues regarding (more or less) minor differences between flow and Eclipse we implicitly state that: "Everything else is implemented to perfection" - that is obviously not the case.

I do not agree with this statement:

the lucky thing is that the number of these kind of keywords is limited

and - actually as flow continues to develop a identity of it's own - the number of such differences might actually increase instead of decrease.

Iff we indeed choose to go in this direction I think the current approach is too verbose - I fear it will not be maintained. Common to all of these checks is is we check an item of a keyword for a specific value, and then report a problem. This should be possible to implement based on a collection of objects resembling:

struct PartiallySupported {
   std::string item;
   std::string item_value;
   std::string msg;
}

and a multimap something like:

std::multi_map<std::string, PartiallySupported> = {{"ENDSCALE" , PartiallySupported("DIRECT" , "DIRECT" , "Not supported ...")}, ....

Then adding a new partially supported keyword would only be to add an entry in this map.

atgeirr · 2016-07-15T07:08:39Z

I have no time now to make a proper review, but I like @joakim-hove's idea, it looks like a good and systematic way to do this.

jokva · 2016-07-18T05:59:58Z

Is there a reason for making this a multimap? It doesn't seem to me like we actually use the properties anyway.

qilicun · 2016-07-18T06:19:33Z

Is there a reason for making this a multimap?

One keyword probably has more than one unsupported option.

jokva · 2016-07-18T06:27:38Z

Didn't notice while skimming. That's fine I suppose.

joakim-hove · 2016-07-18T06:59:43Z

opm/autodiff/MissingFeatures.cpp



 namespace Opm {

 namespace MissingFeatures {

+    template<typename T>
+    struct PartiallySupported {
+        int item;


For increased readability I would prefer if you used item names instead - i.e std::string item.

Yeah, that's fine. My concern is that in the json format for these keywords, the options short cut(that is option name) are somehow different to Eclipse Manual, therefor I choose number instead, if you still think the item name is better, I will change.

My concern is that in the json format for these keywords, the options short cut(that is option name) are somehow different to Eclipse Manual

Yes - I see your point - I would still prefer that you use the string names:

Feel free to submit PR to parser changing the names to the Eclipse names.

Please use the compiled in named constants (we should be better at using these ...):

#include <opm/parser/eclipse/Parser/ParserKeywords/C.hpp> string_options = { {ParserKeywords::COMPORD::keywordName , PartiallySupported<std::string>{1, ParseKeywords::COMPORD::TRACK::itemName}},

With some template trickery I guess it should be possible just use ParserKeywords::COMPORD and ParserKeywords::COMPORD::TRACK

Untested suggestion:

template <typename Keyword, typename Item,typename T> void addUnsupported(std::multimap<std::string, PartiallySupported<T>& map, T itemValue) { std::pair<std::string,PartiallySupported<T>> pair( Keyword::keywordName , PartiallySupported<T>{ Item::itemName , itemValue}); map.insert( pair ); } addUnsupported<ParserKeywords::COMPORD , ParseKeywords::COMPORD::TRACK>( string_options , "TRACK");

}

joakim-hove · 2016-07-20T17:24:22Z

Ok - this looks fine to me. I'll merke tomorrow.

joakim-hove · 2016-07-20T17:29:03Z

opm/autodiff/MissingFeatures.cpp

@@ -55,6 +89,14 @@ namespace MissingFeatures {
            "VAPPARS", "VISCREF", "WATVISCT",
            "WPAVE", "WPIMULT", "WPITAB", "WTEMP",
            "WTEST", "WTRACER", "ZIPPY2" };
+        std::multimap<std::string, PartiallySupported<std::string> > string_options;


I would guess you could skip the last type in angular braces, since that can be inferred.

The entire thing can be brace initialised.

@joakim-hove

If you mean the last type in this line, I guess the answer is no, since the compiler reports error.
addUnsupported<ParserKeywords::COMPORD, ParserKeywords::COMPORD::ORDER_TYPE, std::string>

The entire thing can be brace initialised.

Yes, it is.

I would guess you could skip the last type in angular braces, since that can be inferred.

Not in the constructor unless you give it something to infer from.

jokva · 2016-07-20T18:51:21Z

Clumsy syntax aside, I'm still not convinced this is a good feature at all.

atgeirr · 2016-08-19T09:25:18Z

I am now going to finish review and merging of this PR, sorry for ignoring it for so long! I have discussed some aspects with @joakim-hove, and will also make some comments in the diff.

atgeirr · 2016-08-19T09:25:57Z

opm/autodiff/MissingFeatures.cpp

@@ -55,6 +89,14 @@ namespace MissingFeatures {
            "VAPPARS", "VISCREF", "WATVISCT",
            "WPAVE", "WPIMULT", "WPITAB", "WTEMP",
            "WTEST", "WTRACER", "ZIPPY2" };
+        std::multimap<std::string, PartiallySupported<std::string> > string_options;
+        std::multimap<std::string, PartiallySupported<int> > int_options;
+        addUnsupported<ParserKeywords::COMPORD, ParserKeywords::COMPORD::ORDER_TYPE, std::string>(string_options , "TRACK");


Actually, it is the DEPTH option that is unsupported, not the TRACK option.

Here it means only TRACK is supported.

I realize that now, and see that I should have read the code a bit more carefully. I think that must change: addUnsupported could really be called addSupported! However, instead of maintaining a list of supported features it is better I think to have a list of unsupported parts, so that addUnsupported() works as I initially assumed. Then the checking loop on line 56 should use == instead of != and of course the message must be rewritten, and the calls change to supply the complement of the current options (unsupported parts instead of supported parts).

If you do not agree with that (perhaps there are many more unsupported than supported options for this keyword?) then we must rename addUnsupported->addSupported.

perhaps there are many more unsupported than supported options for this keyword?

This is the main reason why we check supported options, I will rename the function.

atgeirr · 2016-08-19T10:55:32Z

I only found one thing that must change (TRACK->DEPTH) above, otherwise this is ready to be merged. Thanks!

qilicun · 2016-08-22T01:43:13Z

I only found one thing that must change (TRACK->DEPTH) above,

That's a long time, I almost forgot it means only the options listed is supported, will revert, sorry for the noise.

atgeirr · 2016-08-22T08:30:19Z

Ok, I am satisfied with this and will merge when Jenkins says it's ok.

atgeirr · 2016-08-22T08:30:28Z

jenkins build this please

atgeirr · 2016-08-22T13:49:43Z

Ert fails for Jenkins, is that related to ongoing changes @akva2?

akva2 · 2016-08-22T13:53:53Z

yes

akva2 · 2016-08-22T14:20:44Z

and i'm done with the pr for all modules. usual rules apply (no self-merge), cause somebody will likely jump on me if i do.

qilicun · 2016-08-23T01:38:20Z

jenkins build this please

atgeirr · 2016-08-23T06:07:25Z

Travis says ok after recent updates, so in it goes. Thanks!

handle partially supported options manually.

776ab45

treat partially supported options by different multimap.

a9e51ee

joakim-hove reviewed Jul 18, 2016
View reviewed changes

Treat unsupported options in a more general way.

b62834f

joakim-hove reviewed Jul 20, 2016
View reviewed changes

change message type from error to warning.

4ae5a0a

atgeirr reviewed Aug 19, 2016
View reviewed changes

qilicun force-pushed the messages-for-keywords-options branch from c30c295 to 4ae5a0a Compare August 22, 2016 02:03

Rename addUnsupported to addSupported.

95ab703

atgeirr merged commit 183d2f4 into OPM:master Aug 23, 2016

qilicun deleted the messages-for-keywords-options branch August 23, 2016 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle partially supported options manually. #762

Handle partially supported options manually. #762

qilicun commented Jul 14, 2016

joakim-hove commented Jul 14, 2016 •

edited

Loading

atgeirr commented Jul 15, 2016

jokva commented Jul 18, 2016

qilicun commented Jul 18, 2016 •

edited

Loading

jokva commented Jul 18, 2016

joakim-hove Jul 18, 2016

qilicun Jul 18, 2016

joakim-hove Jul 18, 2016

joakim-hove Jul 18, 2016

joakim-hove commented Jul 20, 2016

joakim-hove Jul 20, 2016

jokva Jul 20, 2016

qilicun Jul 21, 2016

qilicun Jul 21, 2016

jokva Jul 21, 2016

jokva commented Jul 20, 2016

atgeirr commented Aug 19, 2016

atgeirr Aug 19, 2016

qilicun Aug 22, 2016

atgeirr Aug 22, 2016

qilicun Aug 22, 2016

atgeirr commented Aug 19, 2016

qilicun commented Aug 22, 2016 •

edited

Loading

atgeirr commented Aug 22, 2016

atgeirr commented Aug 22, 2016

atgeirr commented Aug 22, 2016

akva2 commented Aug 22, 2016

akva2 commented Aug 22, 2016

qilicun commented Aug 23, 2016

atgeirr commented Aug 23, 2016

Handle partially supported options manually. #762

Handle partially supported options manually. #762

Conversation

qilicun commented Jul 14, 2016

joakim-hove commented Jul 14, 2016 • edited Loading

atgeirr commented Jul 15, 2016

jokva commented Jul 18, 2016

qilicun commented Jul 18, 2016 • edited Loading

jokva commented Jul 18, 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

joakim-hove commented Jul 20, 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

jokva commented Jul 20, 2016

atgeirr commented Aug 19, 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

atgeirr commented Aug 19, 2016

qilicun commented Aug 22, 2016 • edited Loading

atgeirr commented Aug 22, 2016

atgeirr commented Aug 22, 2016

atgeirr commented Aug 22, 2016

akva2 commented Aug 22, 2016

akva2 commented Aug 22, 2016

qilicun commented Aug 23, 2016

atgeirr commented Aug 23, 2016

joakim-hove commented Jul 14, 2016 •

edited

Loading

qilicun commented Jul 18, 2016 •

edited

Loading

qilicun commented Aug 22, 2016 •

edited

Loading