Implemented new kernel interface for flux by francoishamon · Pull Request #1696 · GEOS-DEV/GEOS

francoishamon · 2021-11-24T21:12:56Z

This PR implements the new kernel interface for the flux in CompositionalMultiphaseFVM. For now, everything is isothermal, although I left a commented out placeholder to instantiate the kernel class in the thermal case.

francoishamon · 2021-11-24T21:15:02Z

+      m_presAccessor = elemManager.constructArrayViewAccessor< real64, 1 >( keys::pressureString() );
+      m_presAccessor.setName( solverName + "/accessors/" + keys::pressureString() );
+      m_pres = m_presAccessor.toNestedViewConst();


@klevzoff @CusiniM @rrsettgast I have a question about the elementViewAccessors in the new kernel interface.

My initial plan was to move the elementViewAccessors from the solver (ultimately removing CompositionalMultiphaseBase::resetViews) to the kernel, because I wanted to let the kernel choose which elementViewAccessors it needs. This is what is done in the current version of the PR, in which both the elementViewAccessors and the corresponding views are created in the constructor of the kernel interface (see highlighted code).

This plan works well for the CPU, but does not work on the GPU because the elementViewAccessors don't go to the GPU and make the code crash if they are stored in the kernel interface. Is there any way to keep this organization, but make it work on the GPU? Maybe the kernel factory can create the elementViewAccessors?

Alternatively, I can just leave the elementViewAccessors in the solver, and pass the views to the kernel factory, which will solve the problem on the GPU.

…eBase

… API for trait-based access to data.

klevzoff · 2021-12-16T04:22:54Z

+ * @tparam TRAITS the pack containing the types of the fields
+ */
+template< typename ... TRAITS >
+class StencilAccessors


@francoishamon I moved this class into a separate file because ElementRegionManager is already pretty bloated, and this kind of storage class seems quite specific to the stencil-based physics solvers (maybe fluidFlow is too specific a place for it... we can move it later if we want). I also ended up combining StencilAccessors and StencilMaterialAccessors into one to avoid duplicating the code for accessing the data - it just has two distinct constructors, one for regular data and one for material.

klevzoff · 2021-12-16T04:30:26Z

+ * @brief Define the interface for the assembly kernel in charge of flux terms
+ */
+template< integer NUM_COMP, integer NUM_DOF, typename STENCILWRAPPER >
+class FaceBasedAssemblyKernel : public FaceBasedAssemblyKernelBase


Here I extracted a base class FaceBasedAssemblyKernelBase (above) which holds all the data independent of the template parameters, including all field accessors. The reason for that is that the constructor is quite large and involves a lot of template instantiations (first, the machinery of traits::type_list_index<> and then std::get<>) and I'd like to keep it in a separate translation unit (CompositionalMultiphaseFVMKernels.cpp) and also avoid it being recompiled for every combination of template parameters here.

klevzoff · 2021-12-16T04:34:38Z

-                                       m_dPhaseCompFrac_dComp.toNestedViewConst(),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::phaseMobility{} ),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::dPhaseMobility_dPressure{} ),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::dPhaseMobility_dGlobalCompDensity{} ),


This is the new way of extracting a specific accessor, hopefully pretty clean. We can also add using namespace extrinsicMeshData somewhere to make it even shorter.

I only catching up with your modifications now, they look great! The getter for the accessor is now much better than in the previous version of the PR. Thanks!

In addition to using namespace, we could also change extrinsicMeshData to something shorter as discussed previously (in a separate PR). I still think it is a good idea because extrinsicMeshData is quite long to type.

@francoishamon @klevzoff Would it be possible to just pass in the accessors and do the extraction inside the kernel? It seems that the kernels define the accessor types, so they could have knowledge of that type?

Isn't it what the crateAndLaunch does? Maybe that has not been created for the hybridFVM Kernel?

You're both right, this should be done in the kernel class, which has not been created for hybrid solver yet because @francoishamon is going to address it in a future PR

Yes, this needs to be done in a separate PR that will also take care of the aquifer boundary condition kernel, and the CFLFlux kernel. I did not want to change too many things in this PR before making sure that everyone was ok with the new style.

klevzoff

LGTM!

francoishamon · 2021-12-16T21:16:27Z

@CusiniM @rrsettgast and anyone interested, if you have time to look at this PR, it is now ready for review.

francoishamon · 2021-12-21T17:24:59Z

@rrsettgast @klevzoff @CusiniM
I used Caliper to measure the time spent constructing the accessors in the flux kernel factory at each nonlinear iteration:

GEOSX_MARK_BEGIN(StencilAccessorCreation);                                                                                                                                                                
using KERNEL_TYPE = FaceBasedAssemblyKernel< NUM_COMP, NUM_DOF, STENCILWRAPPER >;                                                                                                                         
typename KERNEL_TYPE::CompFlowAccessors compFlowAccessors( elemManager, solverName );                                                                                                                     
typename KERNEL_TYPE::MultiFluidAccessors multiFluidAccessors( elemManager, solverName, targetRegionNames, fluidModelNames );                                                                             
typename KERNEL_TYPE::CapPressureAccessors capPressureAccessors( elemManager, solverName, targetRegionNames, capPresModelNames );                                                                         
typename KERNEL_TYPE::PermeabilityAccessors permeabilityAccessors( elemManager, solverName, targetRegionNames, permeabilityModelNames );                                                                  
GEOSX_MARK_END(StencilAccessorCreation);

I used the coarse version of the ECP stretch problem (2 target regions, 806k cells, 18 MPI ranks, 33 time steps, 234 nonlinear iterations). I got the following results (main kernels only):

Total simulation time: 359 s

Property updates:

                                               Min time/rank Max time/rank Avg time/rank Time %
updatePhaseMobility                               0.507911      0.796943      0.676077  0.188469                                                                 
updateRelPermModel                                0.256718      0.346445      0.311781  0.086915                                                                 
updatePhaseVolumeFraction                         0.366245      0.601683      0.499774  0.139321                                                                 
updateFluidModel                                  8.710323     11.449307     10.463562  2.916909                                                                 
updateComponentFraction                           0.128919      0.206831      0.166747  0.046484                                                                 
updatePorosityAndPermeability                     0.116505      0.149726      0.136339  0.038007

Assembly (including StencilAccessorCreation):

assembleFluxTerms                                37.325503     52.451860     45.788265 12.764314                                                                 
StencilAccessorCreation                           0.517961      0.554279      0.526745  0.146840                                                                 
assembleAccumulationAndVolumeBalanceTerms         1.729113      2.986959      2.419470  0.674471

Solve (including CopyJacobian):

solveSystem                                    245.512842    247.836020    246.054756 68.592251
CopyJacobian                                    26.587304     26.633028     26.609845  7.370147

rrsettgast

This looks great. Just a couple of questions.

rrsettgast · 2021-12-21T18:20:55Z

-                                       m_dPhaseCompFrac_dComp.toNestedViewConst(),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::phaseMobility{} ),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::dPhaseMobility_dPressure{} ),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::dPhaseMobility_dGlobalCompDensity{} ),


@francoishamon @klevzoff Would it be possible to just pass in the accessors and do the extraction inside the kernel? It seems that the kernels define the accessor types, so they could have knowledge of that type?

rrsettgast · 2021-12-21T18:23:23Z

   */
  GEOSX_HOST_DEVICE
-  integer getElemGhostRank( localIndex const ei ) const
+  integer elemGhostRank( localIndex const ei ) const


can we just get rid of this accessor and use the member?

Yes, we can get rid of that. I am taking note of this and will remove in the next PR.

CusiniM · 2021-12-21T19:08:25Z

+   * @param[in] regionNames the name of the solver target regions
+   * @param[in] materialNames the name of the solver material names
+   */
+  StencilAccessors( ElementRegionManager const & elemManager,


We are gonna have to change this interface after the multiple meshBody PR goes in (or in that exact PR) coz both materialNames and regionNames won't be available any more. At least not the way they are now.

CusiniM

This looks good to me. It will be painful to merge with the other PR and we will need to change a few things but we will deal with it.

CusiniM · 2021-12-21T19:29:49Z

-                                       m_dPhaseCompFrac_dComp.toNestedViewConst(),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::phaseMobility{} ),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::dPhaseMobility_dPressure{} ),
+                                       compFlowAccessors.get( extrinsicMeshData::flow::dPhaseMobility_dGlobalCompDensity{} ),


Isn't it what the crateAndLaunch does? Maybe that has not been created for the hybridFVM Kernel?

francoishamon · 2021-12-21T21:33:01Z

Sorry, I am doing one last commit to fix a compilation warning on Lassen. I checked the integrated tests on Quartz and Lassen, no diffs, except the usual ones due to integer types on Lassen. We can merge the PR when the CI tests are done. Thanks @klevzoff for all the help on this PR!

implemented new kernel interface for flux

a912185

francoishamon self-assigned this Nov 24, 2021

francoishamon commented Nov 24, 2021

View reviewed changes

francoishamon changed the title ~~implemented new kernel interface for flux~~ Implemented new kernel interface for flux Nov 25, 2021

francoishamon mentioned this pull request Nov 30, 2021

Implemented new kernel interface for compositional properties #1698

Merged

Merge branch 'develop' into feature/hamon/fluxKernelInterface

44ae09f

francoishamon marked this pull request as ready for review December 9, 2021 16:36

Francois Hamon added 3 commits December 9, 2021 08:37

post-merge fixes

cf7a180

added kernel accessor storage for regular fields

0bd65b8

added kernel accessor storage for material fields

0e751ca

francoishamon mentioned this pull request Dec 13, 2021

Implemented extrinsic data for multiphase constitutive models #1709

Merged

Francois Hamon added 2 commits December 13, 2021 16:55

removed all element view accessor members from CompositionalMultiphas…

5051a39

…eBase

fixed compilation error in ExtrinsicData

9ba400f

francoishamon added the type: cleanup / refactor Non-functional change (NFC) label Dec 15, 2021

Francois Hamon and others added 3 commits December 14, 2021 19:51

Merge branch 'develop' into feature/hamon/fluxKernelInterface

137c070

flux kernel interface passes integrated tests with stencil accessors

b301c98

Move StencilAccessors to a separate file, make it a class and provide…

dc73151

… API for trait-based access to data.

klevzoff reviewed Dec 16, 2021

View reviewed changes

klevzoff approved these changes Dec 16, 2021

View reviewed changes

francoishamon added the flag: ready for review label Dec 16, 2021

francoishamon requested review from CusiniM and rrsettgast December 16, 2021 21:16

francoishamon added flag: ready to be merged and removed flag: ready for review labels Dec 17, 2021

Merge branch 'develop' into feature/hamon/fluxKernelInterface

a78ce23

rrsettgast approved these changes Dec 21, 2021

View reviewed changes

CusiniM reviewed Dec 21, 2021

View reviewed changes

CusiniM approved these changes Dec 21, 2021

View reviewed changes

removed compilation warning on Lassen; integrated tests passing

0d8f8cc

francoishamon merged commit 382c559 into develop Dec 21, 2021

francoishamon deleted the feature/hamon/fluxKernelInterface branch December 21, 2021 23:46

francoishamon mentioned this pull request Jan 14, 2022

Add energy balance equation to Compositional solver. #1225

Merged

2 tasks

Uh oh!

Conversation

francoishamon commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klevzoff left a comment

Choose a reason for hiding this comment

Uh oh!

francoishamon commented Dec 16, 2021

Uh oh!

francoishamon commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rrsettgast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CusiniM Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CusiniM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoishamon commented Dec 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

francoishamon commented Nov 24, 2021 •

edited

Loading

francoishamon commented Dec 21, 2021 •

edited

Loading

CusiniM Dec 21, 2021 •

edited

Loading