Change how the blackoilmodules are setup #725
Conversation
…tate instead add a method to provide parameters externally. this decouples the class from the structures in opm-common, and at the same time adds the ability to use the classes without an EclipseState, making the code more reusable.
…tate instead add a method to provide parameters externally. this decouples the class from the structures in opm-common, and at the same time adds the ability to use the classes without an EclipseState, making the code more reusable.
instead add a method to provide parameters externally. this decouples the class from the structures in opm-common, and at the same time adds the ability to use the classes without an EclipseState, making the code more reusable.
instead add a method to provide parameters externally. this decouples the class from the structures in opm-common, and at the same time adds the ability to use the classes without an EclipseState, making the code more reusable.
…eState instead add a method to provide parameters externally. this decouples the class from the structures in opm-common, and at the same time adds the ability to use the classes without an EclipseState, making the code more reusable.
…eState instead add a method to provide parameters externally. this decouples the class from the structures in opm-common, and at the same time adds the ability to use the classes without an EclipseState, making the code more reusable.
|
jenkins build this opm-simulators=4122 please |
atgeirr
left a comment
atgeirr
left a comment
There was a problem hiding this comment.
Overall, I like this. It is good to put the init code in a cpp file to avoid recompilation. I agree that it is losing something by taking the initialization out of the class, but overall I am in favor.
However, I do not understand why the code should move to opm-simulators. Would it not have been better to keep it here in opm-models? Then the concern over splitting it is less.
| */ | ||
| static void initFromState(const EclipseState& eclState) | ||
| //! \brief Set the parameters. | ||
| static void setParams(BlackOilExtboParams<Scalar> params) |
There was a problem hiding this comment.
Consider taking rvalue ref argument here. As written, we will have minimal overhead (one extra move) when called with a temporary, as it is used downstream, but if someone calls this with an lvalue it will incur a copy. The copy can be avoided if the user remembers to std::move() the argument in the call to this function, but making the argument an rvalue ref will force it, avoiding any chance of copying. (If you actually need to keep the original at the call site, you would need to make a temporary copy there.)
2 participants
remove member function for initing from EclipseState
instead add a method to provide parameters externally. this decouples the classes from the structures in opm-common,
and at the same time adds the ability to use the classes without an EclipseState, making the code more reusable.
i'm not sure if this is a good idea or not. i like the lessening of headers pulled in, and the ability to build this code once.
i dislike moving the configuration out of the classes themself.