Support JFUNC in TableManager #956
Conversation
it looks to me like the respective columns of the underlying 2p tables are still accessed via Also, it would be cool if you would test this on "model 2.2" with flow by running it for a few time steps... |
|
@andlaus I added the |
I'd say "yes" because it is better to get no result than a wrong one!? I guess @joakim-hove will have to decide this in the end.. |
|
@andlaus He told me that your will is my command. Could you check the two last commits of this PR and see if they're ok by you? We should coordinate the merging of this with the necessary changes in material as well. |
|
now I'm happy with this :) please do not merge just yet, I still need to fix up opm-material... |
|
Great! Thanks for reviewing. Just merge this at will. |
* Implement JFUNC keyword * 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
* Implemented missing method "name" in TableColumn.cpp * add inline to method in RestartConfig to remove warning * Whitespace changes
|
Note, I squashed and reordered commits. |
| @@ -60,11 +60,15 @@ namespace Opm { | |||
| */ | |||
| double evaluate(const std::string& columnName, double xPos) const; | |||
|
|
|||
| /// throws std::invalid_argument if jf != m_jfunc | |||
| void assertJFuncPressure(const bool jf) const; | |||
There was a problem hiding this comment.
I just noticed this now while trying to adapt opm-material: I'm pretty sure that this method is specific to (some of) the saturation function tables, i.e., it may be a bit more convenient to add it to the base class of all tables from an implementor's point of view, but it does not make too much sense IMO. do you mind moving this to SwofTable and friends?
There was a problem hiding this comment.
you think it's better to have this function in five different classes than in one superclass? What about the m_jfunc boolean flag? Should that go to the five classes as well?
There was a problem hiding this comment.
I want to unify all the tables under one non-virtual class soon, and have them be distinguished by content. I don't think it's too unreasonable the way it is now for that reason.
If there's a compelling reason to maintain strictly different types for this I'll take it to heart and consider having multiple table types stored in the manager (in the future).
It's on the ever-growing TODO list.
There was a problem hiding this comment.
to all of your questions: yep, since IMO it makes about zero sense for e.g., PvtoTable. alternatively, you can add a SatfuncSimpleTable but I think adding this to all satfunc tables individually is better because this is ECL and it might be that this JFUNC handling does apply to all of them.
There was a problem hiding this comment.
I want to unify all the tables under one non-virtual class soon, and have them be distinguished by content. I don't think it's too unreasonable the way it is now for that reason.
personally I'm not a big fan of "fat interface" approaches: they sometimes might be simpler to implement and require less code, but they are also often very easy to misuse in subtle ways. (e.g. calling pvtoTable.assertJFuncPressure(true) may sometimes work but sometimes not.)
note that I nowadays consider myself just to be a user of opm-parser in 98% of all cases, so if you do it that way anyway, I won't object as long as it works as advertised.
There was a problem hiding this comment.
personally I'm not a big fan of "fat interface" approaches:
It's mostly driven by wanting reasonable value+copy semantics, meaning pointers are clumsy. We'll obviously take care to not make the tables unusable.
|
hm, another problem with this patch that I've just noticed: while it throws at constructs like |
|
yeah, that should be fixed. |
|
hm, I hacked around to adapt opm-material for this yesterday and I changed my opinion during this exercise: with the context dependent pressure columns, the opm-material code just became more complicated, but not better (except -- maybe -- from a conceptual point of view). I guess we should thus consider to leave things as they are now... |
|
Closing this as per discussion. I will cherry-pick the internalization of the keyword (into TableManager) and the improved test at a later point, and come with a new PR on that. |
|
Thanks for reviewing, Jørgen and Andreas ... in any case, we learnt something from this, so it is not entirely wasted time. |
With this PR,
JFUNCkeyword is internalized inTableManagerby the function.useJFunc()JFUNCwill not be SI converted when tables are constructed:SWFN,SGFN,SWOF,SGOF,SLGOFNotice that the last of the three commits only contain general maintenance.