-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat: feature/byer3/thermal well #3156
Conversation
…fix error message for negative mass rate
…or thermal. Enabled kernels/ computes to be thermal aware via specialization or constexpr on template parameter. Most well terms have dt ders. Code tested on 3 standard isothermal models to smoke out offset errors. Lots of asserts left in code to ensure general storage arrays are correct eg.. (dProp[PresIndex]==dprop_dp).. these along with dprop_dp, dprop_dcomp etc arrays will be removed after most testing. Next step to add energy balance to well element flows and perf inflows
…coupling 3) residualnomr calcs, 4) some isothermal , 5) refactoring 6) next focus solution updating/scaling
…al flux index bug 3) 2 isothermal models tested with refactored code 4) thermal case crashes at 2nd linear solve well internal energy derivative = 0
…inear solve well methods 3) next step verify simple test case results
…ng 3) removed debug std::couts 4) next steps a) address constant temp BC b) singlephase thermo dev c) deriv checker
…l derivative checker and treating warnings as errors
…s for shutin well 3) call well initialization code for wells that come online at t> 0
currentBHP = pres[iwelemRef] + totalMassDens[iwelemRef] * diffGravCoef; | ||
dCurrentBHP_dPres = 1 + dTotalMassDens_dPres[iwelemRef] * diffGravCoef; | ||
for( integer ic = 0; ic < numComp; ++ic ) | ||
//integer constexpr NUM_COMP = NC(); |
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.
remove commented
isothermalCompositionalMultiphaseBaseKernels:: | ||
KernelLaunchSelectorCompTherm< PressureRelationKernel >( numFluidComponents(), isThermal, | ||
//isothermalCompositionalMultiphaseBaseKernels::KernelLaunchSelector1< | ||
// PressureRelationKernel >( numFluidComponents(), |
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.
remove commented
src/coreComponents/physicsSolvers/fluidFlow/wells/CompositionalMultiphaseWellFields.hpp
Outdated
Show resolved
Hide resolved
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.
Let's start with this.
src/coreComponents/unitTests/fluidFlowTests/testCompFlowUtils.hpp
Outdated
Show resolved
Hide resolved
src/coreComponents/unitTests/wellsTests/testThermalReservoirCompositionalMultiphaseSSWells.cpp
Outdated
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/fluidFlow/wells/PerforationFluxKernels.hpp
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/fluidFlow/wells/PerforationFluxKernels.hpp
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/fluidFlow/wells/CompositionalMultiphaseWellFields.hpp
Outdated
Show resolved
Hide resolved
@@ -2445,6 +2454,54 @@ void KernelLaunchSelector2( integer const numComp, integer const numPhase, ARGS | |||
} | |||
} | |||
|
|||
template< typename KERNELWRAPPER, typename ... ARGS > | |||
void KernelLaunchSelector_NC_NP_THERM( integer const numComp, integer const numPhase, integer const isThermal, ARGS && ... args ) |
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.
why did you have to add this? didn't we already have multiphase compostional thermal kernels?
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.
is this ever used? I can't find it...
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.
It seems that we have too many of these dispatches. We should only have 1 dispatch for compNumber, phaseNumber and thermal and use it everywhere.
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.
will delete... yes we have several dispatches..In the spirit of " too many" 1) derivative offset structs... since prop derivatives have thermal slot at index 1. traits classes templated on thermal are needed in isothermal runs... where should we put these structs... . I think these and dispatches should all be in some common location so the developer knows what is available... 2) further single/multiphase should be combined, and 3) specializations should only be for different variable sets or implicitness and phase comp partitioning arrays introduced ....
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.
why not using the ones you created?
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.
In. KernelLaunchSelector2 there is a comment // Ideally this would be inside the dispatch, but it breaks on Summit with GCC 9.1.0 and CUDA 11.0.3. I thought it would be better to make that change in different PR.
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.
if this file sits in common it should be a lot more generic IMO.
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 move to fluidflow...
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.
moved to physicsSolvers
src/coreComponents/physicsSolvers/fluidFlow/IsothermalCompositionalMultiphaseBaseKernels.hpp
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/fluidFlow/IsothermalCompositionalMultiphaseBaseKernels.hpp
Show resolved
Hide resolved
@CusiniM @rrsettgast can we please merge it? bunch of useful changes for wells here |
I had left some comments and if they were addressed then I am fine with merging it. However, some things in this PR are done in a way that it's quite different from the rest of the code so I wanted someone else to have a close look at it too. |
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 am not sure I would have put those dispatch functionalities in common/internal. Otherwise this PR looks good to me now
* @detail if isThermal is true, the constitutive model compute the enthalpy and internal energy of the phase. | ||
* This can be used to check the compatibility of the constitutive model with the solver | ||
*/ | ||
virtual bool isThermal() const { return false; } |
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.
@tjb-ltk Is this resolved?
@@ -599,7 +601,7 @@ class ScalingForSystemSolutionKernel : public isothermalCompositionalMultiphaseB | |||
{ | |||
// compute the change in temperature | |||
real64 const temp = m_temperature[ei]; | |||
real64 const absTempChange = LvArray::math::abs( m_localSolution[stack.localRow + m_numComp + 1] ); | |||
real64 const absTempChange = LvArray::math::abs( m_localSolution[stack.localRow + m_temperatureOffset] ); |
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 change looks strange to me...is this OK?
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.
Agree. the change was needed so that wells could use the scaling kernel implemented for the reservoir. The location of the temperature variable is different for well and reservoir, hence the offset variable. It's ok but fragile. Another PR could see this being replaced with a traits class, but defining/changing lineups with traits classes would probably need more vetting beyond this kernel.
@@ -0,0 +1,592 @@ | |||
/* |
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 changes here don't seem well related...or thermal related. I missed it? Can you explain?
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.
Originally the well-res coupling derivatives were not in kernel classes but assembled in CompositionalMultiphaseReservoirAndWells.cpp. With the addition of thermal ,kernels were added for iso + thermal and are in CoupledReservoirAndWellKernels.hpp
src/coreComponents/unitTests/fluidFlowTests/testCompFlowUtils.hpp
Outdated
Show resolved
Hide resolved
…ll values), quantites only used for reporting
Add thermal well formulation for modeling convection in compositional flow models