Add rsw #3246
Add rsw #3246
Conversation
|
jenkins build this opm-models=763 opm-simulators=4292 please |
1 similar comment
|
jenkins build this opm-models=763 opm-simulators=4292 please |
| @@ -422,6 +438,23 @@ class BlackOilFluidState | |||
| } | |||
| } | |||
|
|
|||
| /*! | |||
| * \brief Return the gas dissulition factor of water [m^3/m^3]. | |||
| @@ -769,9 +815,10 @@ class BlackOilFluidSystem : public BaseFluidSystem<Scalar, BlackOilFluidSystem<S | |||
| } | |||
|
|
|||
| case waterPhaseIdx: | |||
| const LhsEval Rsw(0.0); | |||
There was a problem hiding this comment.
Should we not have a branch here for if (enableDissolvedGasInWater()) { similar to below?
Also: are we not missing a branch for the brine case both here and below?
There was a problem hiding this comment.
Yes. This is a bug. It does not affect exisisting usage, since we compute density from invB when updateding the fluidstate in blackoilintensivequantities.hh.
What do you mean by the "brine case"?
There was a problem hiding this comment.
By brine I mean with salt, but now I do not see why I thought something was missing, it is taken care of with saltConcentration as evaluated on line 756 it seems.
| @@ -968,12 +1039,12 @@ class BlackOilFluidSystem : public BaseFluidSystem<Scalar, BlackOilFluidSystem<S | |||
|
|
|||
| const auto& p = decay<LhsEval>(fluidState.pressure(phaseIdx)); | |||
| const auto& T = decay<LhsEval>(fluidState.temperature(phaseIdx)); | |||
| const auto& saltConcentration = decay<LhsEval>(fluidState.saltConcentration()); | |||
| const auto& saltConcentration = BlackOil::template getSaltConcentration_<ThisType, FluidState, LhsEval>(fluidState, regionIdx); | |||
There was a problem hiding this comment.
Could you explain the reason for this change?
There was a problem hiding this comment.
The new code will allow for a fluidstate without a method called saltConcentration(). There are still some call left to fluidState.saltConcentration(), I only "fixed" the part of the code I was changing anyway. This also align more with how we do it for rs and rv etc.
| @@ -968,12 +1039,12 @@ class BlackOilFluidSystem : public BaseFluidSystem<Scalar, BlackOilFluidSystem<S | |||
|
|
|||
There was a problem hiding this comment.
The doxygen doc for this function is now misleading (line 1030), similarly in line 832.
| @@ -1206,7 +1288,9 @@ class BlackOilFluidSystem : public BaseFluidSystem<Scalar, BlackOilFluidSystem<S | |||
|
|
|||
| case waterPhaseIdx: | |||
| return | |||
| waterPvt_->internalEnergy(regionIdx, T, p, BlackOil::template getSaltConcentration_<ThisType, FluidState, LhsEval>(fluidState, regionIdx)) | |||
| waterPvt_->internalEnergy(regionIdx, T, p, | |||
| BlackOil::template getRsw_<ThisType, FluidState, LhsEval>(fluidState, regionIdx), | |||
There was a problem hiding this comment.
Weird indentation, easy to misread. Also on lines 1350, 1378 and 1428.
| const Evaluation& pressure, | ||
| const Evaluation& /*saltConcentration*/) const | ||
| { | ||
| //TODO: The viscosity does not yet depend on the composition |
There was a problem hiding this comment.
This TODO is not applicable to saturatedViscosity() I think.
| const Evaluation& /*saltConcentration*/) const | ||
| { | ||
| //TODO: The viscosity does not yet depend on the composition | ||
| return saturatedViscosity(regionIdx, temperature, pressure); |
There was a problem hiding this comment.
In this function on the other hand, the TODO applies. But do we want to include the effect of dissolved gas here? Based on some analytic function from some paper, if such a thing exists?
There was a problem hiding this comment.
One candidate is to use the LBC model for co2rich mixture (that we already have in opm/material/viscocitymodels)
| const Evaluation& Rs, | ||
| const Evaluation& /*saltConcentration*/) const | ||
| { | ||
| return inverseFormationVolumeFactor(regionIdx, temperature, pressure, Rs); |
There was a problem hiding this comment.
I am in general a bit worried about adding new methods that are so similar to the existing ones, in terms of interface: forget one parameter, and you may be calling the completely wrong function (or rather, with parameters other than you think). This may apply to the other Pvt classes as well, but I noticed it here. Is this new?
There was a problem hiding this comment.
No it is not introduced here. I think a good solution would be to stop using the pvt models for the phases directly (they should be internal to the FluidSystem) and instead call FluidSystem::density(fs, ...) etc. that would avoid the potential for misuse.
| const Evaluation& saltconcentration) const | ||
| { OPM_WATER_PVT_MULTIPLEXER_CALL(return pvtImpl.inverseFormationVolumeFactor(regionIdx, temperature, pressure, saltconcentration)); | ||
| { OPM_WATER_PVT_MULTIPLEXER_CALL(return pvtImpl.inverseFormationVolumeFactor(regionIdx, temperature, pressure, Rsw, saltconcentration)); |
There was a problem hiding this comment.
Nitpick (here and the next 4 multiplexer calls): Please put the call on the line following the brace, and the return 0 on the line after that by itself, just as for saturatedViscosity().
|
jenkins build this opm-models=763 opm-simulators=4292 please |
1 similar comment
|
jenkins build this opm-models=763 opm-simulators=4292 please |
If DISGASW is in the input deck gas in allowed to dissolved into water Currently only works in combination with CO2STORE For general support assosiated input tables needs to be provided
|
jenkins build this opm-models=763 opm-simulators=4292 please |
4 similar comments
|
jenkins build this opm-models=763 opm-simulators=4292 please |
|
jenkins build this opm-models=763 opm-simulators=4292 please |
|
jenkins build this opm-models=763 opm-simulators=4292 please |
|
jenkins build this opm-models=763 opm-simulators=4292 please |
|
All is green and all is approved. Will merge! Thanks @atgeirr for the review. |
Ported from OPM/opm-material#543