Remove black oil props from init

[totto82]

This PR untangle the EQUIL initialization from opm-core legacy property, blackoilState and blackoilPhase objects.

[bska]

This PR untangle the EQUIL initialization

I'm not sure "untangle" is the word I'd use for this. The word "switch" is definitely more descriptive.

The feature set looks okay, but the implementation is not. I'm not going to merge this in its current state. There are too many shared_ptr<X> parameters that should be const X& instead and rely on nested lifetimes. Moreover, the "always three-phase" behaviour is a step backwards. Why is that necessary? Will we reenable two-phase support at a later time? If not, why not?

Add to this a litany of white-space changes that only obscure the change-set and using ->template outside of dependent names in addition to owning raw pointers that should be smart pointers.

In short, this proposal needs additional work before it is suitable for inclusion.

[totto82]

There are too many shared_ptr<X> parameters that should be const X&

You mean the MaterialLawManager. I just followed what's the practice in the rest of the code.

Moreover, the "always three-phase" behaviour is a step backwards. Why is that necessary? Will we reenable two-phase support at a later time? If not, why not?

It works perfectly for 2p cases. It is just that the size of the pressure and saturations vectors are always 3 in order to use the fixed phaseIdx from opm-material. I don't think this will be very time critical anyway.

Add to this a litany of white-space changes that only obscure the change-set and using ->template outside of dependent names in addition to owning raw pointers that should be smart pointers

I will clean this up.

[bska]

There are too many shared_ptr<X> parameters that should be const X&

You mean the MaterialLawManager.

I do.

I just followed what's the practice in the rest of the code.

Then this is an opportunity to improve upon the existing practice. An interface that accepts a shared_ptr<> explicitly states that it wants to take part in the lifetime management of the underlying object. The majority of this code does not need to do that.

Moreover, the "always three-phase" behaviour is a step backwards. Why is that necessary? Will we reenable two-phase support at a later time? If not, why not?

It works perfectly for 2p cases. It is just that the size of the pressure and saturations vectors are always 3 in order to use the fixed phaseIdx from opm-material. I don't think this will be very time critical anyway.

Is that an internal implementation detail or does it leak out to callers? Always carrying a three-phase saturation vector is definitely not a trivial cost.

[totto82]

Is that an internal implementation detail or does it leak out to callers? Always carrying a three-phase saturation vector is definitely not a trivial cost.

Partly. The caller needs to initializer whatever state object it uses with the correct number of phases. Both Flow and Flow-legacy only carries around the needed phases. (i.e. 2 for 2p problems)

I changed the shared pointers to ref. as you suggested.

and using ->template outside of dependent names in addition to owning raw pointers that should be smart pointers.

Could you be more specific on this?

[bska]

and using ->template outside of dependent names in addition to owning raw pointers that should be smart pointers.

Could you be more specific on this?

I mean that the ->template syntax, or its brethren ::template and .template, exist for the purpose of invoking templated identifiers (member functions, member types &c) on objects that depend on a template parameter. They are not needed to invoke any arbitrary templated member function regardless of context and we generally should not use that syntax outside the implementation of a template (e.g., a function template).

As for owning raw pointers, your initDefaultFluidSystem() function contains multiple instances of

X* p = new X(args);

when

auto p = make_shared<X>(args);

serves the same purpose in a much safer way, especially since the pointers are subsequently put into shared_pointer<> containers anyway. I think you should apply the patch below and, while you're there, also declare the function static (or preferably put it into an anonymous namespace) to address the warnings concerning

"no previous declaration for ‘void initDefaultFluidSystem()’ [-Wmissing-declarations]"

By the way you should also remove the const qualifier in the return type of

const int Opm::EQUIL::EquilReg::pvtIdx() const;

Returning a const int serves no purpose.

---

diff --git a/tests/test_equil.cpp b/tests/test_equil.cpp
index 3c40a287..b41fd0da 100644
--- a/tests/test_equil.cpp
+++ b/tests/test_equil.cpp
@@ -98,25 +98,25 @@ void initDefaultFluidSystem() {
     FluidSystem::setEnableVaporizedOil(false);
     FluidSystem::setReferenceDensities(rhoRefO, rhoRefW, rhoRefG, /*regionIdx=*/0);

-    Opm::GasPvtMultiplexer<double> *gasPvt = new Opm::GasPvtMultiplexer<double>;
+    auto gasPvt = std::make_shared<Opm::GasPvtMultiplexer<double>>();
     gasPvt->setApproach(Opm::GasPvtMultiplexer<double>::DryGasPvt);
-    auto& dryGasPvt = gasPvt->template getRealPvt<Opm::GasPvtMultiplexer<double>::DryGasPvt>();
+    auto& dryGasPvt = gasPvt->getRealPvt<Opm::GasPvtMultiplexer<double>::DryGasPvt>();
     dryGasPvt.setNumRegions(/*numPvtRegion=*/1);
     dryGasPvt.setReferenceDensities(/*regionIdx=*/0, rhoRefO, rhoRefG, rhoRefW);
     dryGasPvt.setGasFormationVolumeFactor(/*regionIdx=*/0, Bg);
     dryGasPvt.setGasViscosity(/*regionIdx=*/0, mug);

-    Opm::OilPvtMultiplexer<double> *oilPvt = new Opm::OilPvtMultiplexer<double>;
+    auto oilPvt = std::make_shared<Opm::OilPvtMultiplexer<double>>();
     oilPvt->setApproach(Opm::OilPvtMultiplexer<double>::DeadOilPvt);
-    auto& deadOilPvt = oilPvt->template getRealPvt<Opm::OilPvtMultiplexer<double>::DeadOilPvt>();
+    auto& deadOilPvt = oilPvt->getRealPvt<Opm::OilPvtMultiplexer<double>::DeadOilPvt>();
     deadOilPvt.setNumRegions(/*numPvtRegion=*/1);
     deadOilPvt.setReferenceDensities(/*regionIdx=*/0, rhoRefO, rhoRefG, rhoRefW);
     deadOilPvt.setInverseOilFormationVolumeFactor(/*regionIdx=*/0, Bo);
     deadOilPvt.setOilViscosity(/*regionIdx=*/0, muo);

-    Opm::WaterPvtMultiplexer<double> *waterPvt = new Opm::WaterPvtMultiplexer<double>;
+    auto waterPvt = std::make_shared<Opm::WaterPvtMultiplexer<double>>();
     waterPvt->setApproach(Opm::WaterPvtMultiplexer<double>::ConstantCompressibilityWaterPvt);
-    auto& ccWaterPvt = waterPvt->template getRealPvt<Opm::WaterPvtMultiplexer<double>::ConstantCompressibilityWaterPvt>();
+    auto& ccWaterPvt = waterPvt->getRealPvt<Opm::WaterPvtMultiplexer<double>::ConstantCompressibilityWaterPvt>();
     ccWaterPvt.setNumRegions(/*numPvtRegions=*/1);
     ccWaterPvt.setReferenceDensities(/*regionIdx=*/0, rhoRefO, rhoRefG, rhoRefW);
     ccWaterPvt.setViscosity(/*regionIdx=*/0, 1);

[totto82]

@bska Thanks for the explanation. I will try it out tomorrow.