Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixing energy #309

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Mixing energy #309

wants to merge 5 commits into from

Conversation

hnil
Copy link
Member

@hnil hnil commented Oct 17, 2018

Modifications to make energy module for black oil to not only gennerate termperature due to the entalpy/work term. This is physical since we assume the energy density only depend on temperature. It is introduced energy density on each phase depending on the phases.

Copy link
Contributor

@andlaus andlaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in principle this is okay. I have a few remarks, though:

  • please use camelCase within this module
  • please do not leave commented out code in the PR, remove it instead
  • what's the difference between the new energy() function in the fluid state and internalEnergy()?
  • is the new function actually used?
  • if yes, did you compare this with E100 or E300?

{ return (*enthalpy_)[phaseIdx] - pressure(phaseIdx)/density(phaseIdx); }
{
//double ent_fac=0.0;
double ent_fac=1.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camelCase please

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in principle this is the same but internalEnergy only forward to a stored entalpy. For eclipse type of models it is probably simpler to do the oposite.

@@ -551,7 +551,7 @@ class BlackOilFluidSystem : public BaseFluidSystem<Scalar, BlackOilFluidSystem<S
if (enableDissolvedGas()) {
// miscible oil
const LhsEval& Rs = Opm::BlackOil::template getRs_<ThisType, FluidState, LhsEval>(fluidState, regionIdx);
const LhsEval& bo = oilPvt_->inverseFormationVolumeFactor(regionIdx, T, p, Rs);
const LhsEval& bo = inverseFormationVolumeFactor(fluidState, phaseIdx, regionIdx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the new version identical to the old one? if no, the Rs variable above should be unused?!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not think so, since since the member function do the bookkeeping with of finding if the fluid is saturated or not. It probably had been better to take all of this directly from the state, since this will ensure consistency with the flow problem which is essential for physical behavoir.. (Else the mass in the energy equation is not consitent with the mass int the system and tempreratur can be gennerated.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could I take it directly form the state. (I tried but broke some tests)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interpolation between the saturated and the undersaturated interpolation should only affect the derivatives, i.e. if there are no convergence issues due to energy, it should not be necessary!?

How could I take it directly form the state. (I tried but broke some tests)

BlackOilFluidState has and invB() method. for generic fluid states this only makes limited sense, so it needs to be calculated. The best technical solution is probably to create something analogous to the getRs_(fluidState) function for inverse formation volume factors:

https://github.com/OPM/opm-material/blob/master/opm/material/fluidsystems/BlackOilFluidSystem.hpp#L52

But I guess that this is too much effort here...

static LhsEval energy(const FluidState& fluidState,
unsigned phaseIdx,
unsigned regionIdx)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that function should be named either internalEnergy() or enthalpy(). Also, internalEnergy() and enthalpy() need to be consistent, i.e. h_alpha = u_alpha + p_alpha/rho_alpha.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed your example and this energy is only used to calculate enthalpy and could probably be made private.

Copy link
Contributor

@andlaus andlaus Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, okay. maybe this can even replace the enthalpy() and/or internalEnergy() functions directly?

+ p/density<FluidState, LhsEval>(fluidState, phaseIdx, regionIdx);
//const auto& T = Opm::decay<LhsEval>(fluidState.temperature(phaseIdx));
//double ent_fac=0;
double ent_fac = 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camelCase please.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for consistency with E100 and some simple investigation it had been use full to have this entfac as parameter. but it need to be consistent between the fluid state and the fluid system in the way. (If entFace = 0, enthalpy effect is ignored and one should temprature should stay within boundary conditions if I am not wrong...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an ECLIPSE parameter, i.e. can it somehow be set from the deck?

gasPvt_->internalEnergy(regionIdx, T, p, Opm::BlackOil::template getRv_<ThisType, FluidState, LhsEval>(fluidState, regionIdx))
+ p/density<FluidState, LhsEval>(fluidState, phaseIdx, regionIdx);
//const auto& T = Opm::decay<LhsEval>(fluidState.temperature(phaseIdx));
//double ent_fac=0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove these two lines.

}

throw std::logic_error("Unhandled phase index "+std::to_string(phaseIdx));
//return energy;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

@andlaus
Copy link
Contributor

andlaus commented Oct 18, 2018

I thought about whether this is physically a good idea and I am now even more unsure: the internal energy for wet gas and live oil should be lower (or maybe higher?) than for the same amounts of pure gas and pure oil. i.e., physically the temperature should change if the two components are mixed together...

@andlaus
Copy link
Contributor

andlaus commented Jan 7, 2019

it seems like this became stale. what's the status here? if you think we should spend time on testing this, please make it mergable again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants