-
Notifications
You must be signed in to change notification settings - Fork 97
Use of CompressibleSolid - Stencils refactoring - Transmissibility update #1311
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
Use of CompressibleSolid - Stencils refactoring - Transmissibility update #1311
Conversation
…ionalFlowInFractures
…ionalFlowInFractures
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.
Solid work! Haven't gone through the whole thing yet, but here are a few comments
| struct viewKeyStruct : public ConstitutiveBase::viewKeyStruct | ||
| { | ||
| static constexpr char const * newPorosityString() { return "newPorosity"; } | ||
| static constexpr char const * newPorosityString() { return "porosity"; } |
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 like the change, but can we also rename the function and corresponding member (m_newPorosity)?
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 wanted to keep the same naming convention as for the stress. So, I am ok with changing it but I would then change it for both.
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 seem to vaguely remember a similar discussion happening around stress, indeed. I'm sure there were good reasons to keep the name newStress. For me personally, the new/old monikers only make sense inside the update function; once the update is done, and for the majority of the code, "new" stress/porosity is just the current value - I would maybe actually prefer curr/prev over new/old (though I've introduced a lost of old variables in the past myself). But consistency across models is also important.
|
|
||
| std::vector< string > FlowSolverBase::getConstitutiveRelations( string const & regionName ) const | ||
| { | ||
| // TODO IS THIS EVER USED? WHAT FOR? |
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.
Not used currently. I think the idea was to do more intelligent choice of quadrature dimension for each constitutive model based on whether a FEM or a FV solver uses that model. I'm not sure if we're ever gonna go that route though.
| ElementViewConst< arrayView1d< real64 const > > const & aperture, | ||
| CRSMatrixView< real64, globalIndex const > const & localMatrix, | ||
| arrayView1d< real64 > const & localRhs ) | ||
| launch< EmbeddedSurfaceToCellStencil >( EmbeddedSurfaceToCellStencil const & GEOSX_UNUSED_PARAM( stencil ), |
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 don't think all these specializations are necessary, since you've limited the dispatch only to SurfaceElementStencil?
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 think I had to add them because the compiler was not happy otherwise but I can try to remove them and check again.
src/coreComponents/physicsSolvers/multiphysics/SinglePhaseReservoir.cpp
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.
This looks great and ready to go IMO (as soon as CI is passed)
| * @param jumpDofKey dofKey of the displacement jump | ||
| */ | ||
| virtual void | ||
| assemblePoroelasticFluxTerms( real64 const time_n, |
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.
These things look a bit hacky, but I don't have a concrete refactoring suggestion at the moment. Unless somebody has one, let's just merge as is and refactor later.
…ionalFlowInFractures
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.
Great work! Everything looks very good to me. I just made small comments to help with the cleanup before merging.
src/coreComponents/unitTests/fluidFlowTests/testSinglePhaseFVMKernels.cpp
Outdated
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/multiphysics/HydrofractureSolver.cpp
Outdated
Show resolved
Hide resolved
...hysicsSolvers/fluidFlow/integratedTests/compositionalMultiphaseFlow/deadoil_3ph_corey_2d.xml
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/fluidFlow/SinglePhaseFVMKernels.cpp
Outdated
Show resolved
Hide resolved
src/coreComponents/finiteVolume/EmbeddedSurfaceToCellStencil.hpp
Outdated
Show resolved
Hide resolved
src/coreComponents/finiteVolume/EmbeddedSurfaceToCellStencil.hpp
Outdated
Show resolved
Hide resolved
src/coreComponents/finiteVolume/EmbeddedSurfaceToCellStencil.hpp
Outdated
Show resolved
Hide resolved
src/coreComponents/finiteVolume/EmbeddedSurfaceToCellStencil.hpp
Outdated
Show resolved
Hide resolved
src/coreComponents/constitutive/permeability/ParallelPlatesPermeability.hpp
Show resolved
Hide resolved
| real64 const perm = 0.25 * ( oldHydraulicAperture*oldHydraulicAperture*oldHydraulicAperture + | ||
| oldHydraulicAperture*oldHydraulicAperture*newHydraulicAperture + | ||
| oldHydraulicAperture*newHydraulicAperture*newHydraulicAperture + | ||
| newHydraulicAperture*newHydraulicAperture*newHydraulicAperture ) / 12; |
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.
We should note that this is essentially a choice on integration schemes.
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.
yeah, I think we may want an option here somehow. I have kept this one coz it was used in the previous version of the code and some integratedTests do not converge with the fully nonlinear model. They may just require a smaller timestep and we could just revisit this.
src/coreComponents/constitutive/permeability/PermeabilityBase.hpp
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| GEOSX_HOST_DEVICE | ||
| void updateFromPressureAndAperture( localIndex const k, |
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.
update what from pressure and aperture?
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.
oh, I thought the constitutive model in general and so all models it "contains".
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.
State update?
| * @brief Catalog name | ||
| * @return Static catalog string | ||
| */ | ||
| static string catalogName() { return string( "CompressibleSolid" ) + PORO_TYPE::catalogName() + PERM_TYPE::catalogName(); } |
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.
no variation in poro type?
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.
Not for now since we only have 2 porosity models and one is only for poromechanics which uses PorousSolid. It may change though if, for example, we introduce geochemistry.
| * @return number of gauss points per element | ||
| */ | ||
| GEOSX_HOST_DEVICE | ||
| localIndex numGauss() const { return m_porosityUpdate.numGauss(); } |
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.
Not really happy with explicit calling this "Gauss". It is really number of material points per element, and the fact that they are used in some sort of integration isn't really in scope from the constitutive model point of view....unless I am not thinking about this correctly?
| * @param stiffness the stiffness array | ||
| */ | ||
| GEOSX_HOST_DEVICE | ||
| void getElasticStiffness( localIndex const k, real64 ( & stiffness )[6][6] ) const |
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 called?
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.
yeah, the EFEM kernel needs it. It's not great coz it means it will only work with Elasticity but that's the way it is for now.
| registerWrapper( viewKeyStruct::newPorosityString(), &m_newPorosity ). | ||
| setPlotLevel( PlotLevel::LEVEL_0 ). | ||
| setApplyDefaultValue( 0.2 ); // will be overwritten | ||
| setApplyDefaultValue( 1.0 ); // will be overwritten but it's important for newly created faceElements. |
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.
??
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.
Yeah, when creating a fracture element it is convenient if it's porosity is automatically set to 1.0. Otherwise one would have to set that value. For the cell elements is not a problem coz porosity is properly set usually either through a field specification or by defining the referencePorosity.
In this PR we
CompressibleSolidin the flow solvers.accummulationKernelsto use new and old porosity instead of thepvMultiplier.PoreVolumeCompressibleSolidandPoroElastic.stencilWrappersthat contain only views and can be called in the kernels to update transmissibilityembeddedSurfacesToCellconnections andfaceElementsToCellsones ( Each connection type updates transmissibility in a different way)SinglePhaseFlowFluxKernelsfor all stencil types and allow for transmissibility updateCompositionalFlowSolverand its kernels to work with all subregion and stencil types to have multiphase flow in the fractures.HydrofractureSolverinherit from the poromechanics one.Rebaseline: https://github.com/GEOSX/integratedTests/pull/145