-
Notifications
You must be signed in to change notification settings - Fork 91
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
fix: use mass and energy consistently for single phase solvers #3485
base: feature/byer3/single_phase_prop_refactor
Are you sure you want to change the base?
fix: use mass and energy consistently for single phase solvers #3485
Conversation
@@ -241,46 +241,38 @@ DECLARE_FIELD( temperatureScalingFactor, | |||
NO_WRITE, | |||
"Scaling factors for temperature" ); | |||
|
|||
DECLARE_FIELD( mass, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mass
went to single phase file since it is used there, not for compositional
@@ -594,8 +634,6 @@ void SinglePhaseBase::initializeFluidState( MeshLevel & mesh, arrayView1d< strin | |||
|
|||
// 2. save the initial density (for use in the single-phase poromechanics solver to compute the deltaBodyForce) | |||
fluid.initializeState(); | |||
|
|||
updateMass( subRegion ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this call is redundant, updateMass
already called in updateFluidState few lines above
@@ -291,13 +260,11 @@ class SurfaceElementAccumulationKernel : public AccumulationKernel< SurfaceEleme | |||
void computeAccumulation( localIndex const ei, | |||
Base::StackVariables & stack ) const | |||
{ | |||
Base::computeAccumulation( ei, stack, [&] () | |||
Base::computeAccumulation( ei, stack ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep it simple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice... introducing mass cleaned up accum and flux kernels a fair bit and probably reduced computation at the same time ... same for introducing energy variable..
stack.dSolidEnergy_dPres = dSolidVolume_dPres * m_rockInternalEnergy[ei][0]; | ||
stack.dSolidEnergy_dTemp = solidVolume * m_dRockInternalEnergy_dTemp[ei][0] + dSolidVolume_dTemp * m_rockInternalEnergy[ei][0]; | ||
} | ||
{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could not remove this, somehow compiler gets always confused
you may want to keep an eye on this: #3460 |
Yes thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -257,53 +263,93 @@ void SinglePhaseBase::updateMass( ElementSubRegionBase & subRegion ) const | |||
{ | |||
GEOS_MARK_FUNCTION; | |||
|
|||
using DerivOffset = constitutive::singlefluid::DerivativeOffsetC< 1 >; // TODO check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assumption with this is that someone doesn't make pressure second index in isothermal run... to be safe I would use
geos::internal::kernelLaunchSelectorThermalSwitch( isThermal, [&] ( auto ISTHERMAL ) {
integer constexpr IS_THERMAL = ISTHERMAL();
using DerivOffset = constitutive::singlefluid::DerivativeOffsetC< IS_THERMAL >;
if constexpr(IS_THERMAL) // for the thermal bits
{
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added, thanks!
} | ||
|
||
void SinglePhaseBase::updateEnergy( ElementSubRegionBase & subRegion ) const | ||
{ | ||
GEOS_MARK_FUNCTION; | ||
|
||
using DerivOffset = constitutive::singlefluid::DerivativeOffsetC< 1 >; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by definition this is thermal routine so this is fine
should go after #3460
unified where possible
for poromechanics - hard to do because porosity is being updated inside the assembly kernel, so left it as is
fixed glitch with double porosity update in poromechanics when it was updated both in flow solver and in poromechanics kernel using different equations leading to potential inconsistencies