Parser::parse functions and ParseContext is member of state, allowing for default constructor

[pgdr]

ParseContext is member of state, allowing for default constructor

• ParseContext is now a true member of EclipseState
• ParseContext is copied into EclipseState so we no longer share reference
• Constructor now has default ParseContext argument
• Updated tests to reflect this, removed a test that checked ref equality
• Added static methods for Parser for constructing grid and state
• Added new ParseContext MISSING_SECTIONS with which we cannot construct state
• parse functions take ref over shared_ptr. removed duplicate comment

[joakim-hove]

Thank you - this was interesting. I think it is difficult to find the right balance between explicit (and typically verbose) and simple to use. Without really thinking it through my instinctive reaction is to go for the explicit and verbose solution; and come on - we are paid to do this! Seriously I guess the important question is to what extent it is simple/possible to misunderstand the API and use it incorrectly. If the API can easily be used wrong[1] I think explicit is good.

For the current PR I do not think the API is easily misunderstood; so in this case I guess simplicity should win. But I will let the PR linger here for some time, and invite other developers to voice an opinion on the general matter.

To take it one step further I think this PR should be updated with the same default behavior for the Parser::parseFile( ) and Parser::parseString( ) methods.

[1]: Of course if possible the best thing would be to fix the API.

[joakim-hove]

Inspired by @jokva, and in the same line of simplification - we could create static methods which instantiate and subsequently discard a parser:

Deck Parser::createDeck( const std::string& inputFile , ParseContext parseContext = ParseContext()) {
    Parser parser;
    return parser.parseFile( inputFile , parseContext );
}

EclipseState Parser::createState(const std::string& inputFile , ParseContext parseContext = ParseContext()) {
    auto deck = Parser::createDeck( inputFile , parseContext );
    return EclipseState( deck , parseContext );
}

[joakim-hove]

but the motivation was Atgeirr's comment that to get a grid you need to (1) create deck, (2) create ParseContext, (3) create EclipseState, (4) state.getInputGrid().

Actually it starts with a (0) create parser.

[jokva]

It's pretty pointless to pass something as a const copy (because the copy is local anyway, and modifications won't bubble outwards).

[jokva]

Inspired by @jokva, and in the same line of simplification - we could create static methods which instantiate and subsequently discard a parser:

Free function - they only use the public interface of Parser anyway and its meaningless for it to be a part of parser (for other than namespacing reasons). If that's the intention then it's all fine obviously.

[joakim-hove]

for other than namespacing reasons

Well - the suggestion to use a static method was all about namespacing.

[atgeirr]

(This is to the discussion about simple free or static methods for simplifying opm-parser usage.)

One idea could be to have a facade class for all things the parser creates. Essentially you'd give it the deck filename and ParseContext (with default) as a constructor argument and it creates the Parser, Deck and Eclipsestate. These objects (and the EclipseGrid) are then available by simple accessor methods. It would make for a simple one-stop solution while simultaneously being flexible enough for any application. What do you think?

[jokva]

It doesn't need Parser (which is rather uninteresting once you've produced your Deck. Deck-use too is discouraged for most applications, so a short EclipseState parse( ... ) should do the trick.

[joakim-hove]

@atgeirr: It would make for a simple one-stop solution while simultaneously being flexible enough for any application. What do you think?

@jokva: so a short EclipseState parse( ... ) should do the trick.

I agree with @jokva here - when "the harm is done" we don't really neither the Parser nor the ParseContext; the only products from the parsing process are the Deck and EclipseState - and we are working towards a shiny future were only the EclipseState is needed.

One idea could be to have a facade class for all things the parser creates.

Actually the EclipseState is in my opinion quite close to such a facade.

[atgeirr]

So an EclipseState constructor taking filename and optional ParseContext? That sounds quite nice actually. There are parts of the code that need direct Deck access and could not use this, although the need is reduced for every little part that gets into the EclipseState.

[joakim-hove]

So an EclipseState constructor taking filename and optional ParseContext?

In principle yes; whether it is a static method on the Parser (my favorite), an EclipseState constructor or a free function is an open question though.

[pgdr]

@joakim-hove This is perhaps the most typical example where such a parser function could be nice to have.

[andlaus]

I don't like the name of the method (parse()): IMO this name applies to parsing in-memory data as equally as to parsing files, network resources and god knows what. what's wrong with parseFile(), parseString(), etc?

[pgdr]

Nothing wrong, but the main reason I chose parse was because there are already functions with the names you mention, with the same input parameters, but different return value. One possibility would be to hijack these names, but I think they are very widely used; DeckPtr parseFile and DeckPtr parseString etc.

[andlaus]

I think parse() is a way too generic name (and IMO it is also unclear that it returns an EclipseState while Parser::parse*() returns Deck objects). That said, I nowadays see myself primarily as a downstream user of opm-parser, so I don't have a real problem with it...

[pgdr]

I just want to stress again that this is not ready to be merged and (for the moment) intended for educational purposes.

[joakim-hove]

I just want to stress again that this is not ready to be merged and (for the moment) intended for educational purposes.

Sure. PS: you can update the PR description at the very top!

[jokva]

Since this constructor is new, can we use Deck& over std::shared_ptr< Deck >?

[pgdr]

I am not sure, but I can try. It would imply that EclipseGrid can be constructed from a reference. I don't think it is impossible, but I don't know how far that would propagate. Investigating now.

[pgdr]

EclipseGrid and EclipseState now have constructors accepting references to Deck instead of shared_ptr's.

It took some refactoring; I had to tweak the constructors of NNC, Schedule, and InitConfit to accept Deck& instead of DeckPtr as well.

But all in all, I think this is a good change that we should cherry-pick if we don't want this entire PR.

[andlaus]

Since this constructor is new, can we use Deck& over std::shared_ptr< Deck >?

don't forget to mark it explicit. (that's also a good idea in its current form.)