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

feat: feature/byer3/thermal well #3156

Open
wants to merge 76 commits into
base: develop
Choose a base branch
from
Open

Conversation

tjb-ltk
Copy link
Contributor

@tjb-ltk tjb-ltk commented Jun 3, 2024

Add thermal well formulation for modeling convection in compositional flow models

tjb-ltk and others added 24 commits December 20, 2023 14:48
…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
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 72.91271% with 571 lines in your changes missing coverage. Please review.

Project coverage is 56.88%. Comparing base (b4b1a3a) to head (720901c).

Files Patch % Lines
...ells/ThermalCompositionalMultiphaseWellKernels.hpp 48.29% 152 Missing ⚠️
...rs/multiphysics/CoupledReservoirAndWellKernels.hpp 0.00% 124 Missing ⚠️
...Solvers/fluidFlow/wells/PerforationFluxKernels.hpp 70.63% 74 Missing ⚠️
...rc/coreComponents/common/KernelLaunchSelectors.hpp 25.00% 60 Missing ⚠️
...dFlow/wells/CompositionalMultiphaseWellKernels.hpp 85.80% 43 Missing ⚠️
...ies/ThermalCompositionalMultiphaseReservoirFVM.hpp 0.00% 34 Missing ⚠️
.../physicsSolvers/fluidFlow/wells/WellSolverBase.cpp 55.35% 25 Missing ⚠️
...dFlow/wells/CompositionalMultiphaseWellKernels.cpp 76.47% 16 Missing ⚠️
...ysics/CompositionalMultiphaseReservoirAndWells.cpp 17.64% 14 Missing ⚠️
...ers/fluidFlow/CompositionalMultiphaseHybridFVM.cpp 0.00% 9 Missing ⚠️
... and 7 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3156      +/-   ##
===========================================
+ Coverage    55.99%   56.88%   +0.88%     
===========================================
  Files         1053     1063      +10     
  Lines        89157    90755    +1598     
===========================================
+ Hits         49921    51623    +1702     
+ Misses       39236    39132     -104     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.gitmodules Outdated Show resolved Hide resolved
* @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; }
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to add one with true in ThermalCompressibleSinglePhaseFluid ?

Copy link
Member

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?

Copy link
Contributor

@victorapm victorapm left a comment

Choose a reason for hiding this comment

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

The linear solver changes look good. Just need to update the header comment for the new MGR strategy

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();
Copy link
Contributor

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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented

Copy link
Collaborator

@CusiniM CusiniM left a 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.

@@ -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 )
Copy link
Collaborator

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?

Copy link
Collaborator

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...

Copy link
Collaborator

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.

Copy link
Contributor Author

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 ....

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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...

Copy link
Contributor Author

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/dataRepository/Group.hpp Outdated Show resolved Hide resolved
@paveltomin
Copy link
Contributor

@CusiniM @rrsettgast can we please merge it? bunch of useful changes for wells here

@CusiniM
Copy link
Collaborator

CusiniM commented Oct 9, 2024

@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.

Copy link
Collaborator

@CusiniM CusiniM left a 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

@tjb-ltk tjb-ltk added flag: requires rebaseline Requires rebaseline branch in integratedTests and removed flag: ready for review labels Oct 10, 2024
* @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; }
Copy link
Member

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] );
Copy link
Member

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?

Copy link
Contributor Author

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 @@
/*
Copy link
Member

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?

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: requires rebaseline Requires rebaseline branch in integratedTests type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants