GEOS-DEV / GEOS

GEOS Simulation Framework
GNU Lesser General Public License v2.1
206 stars 83 forks source link

Initialization step for physics solvers #2178

Open CusiniM opened 1 year ago

CusiniM commented 1 year ago

Describe the issue We currently have a few different functions that can be used for initialization of the physics solvers. The role of each one of them is not always clear so physics solvers tend to use them a bit inconsistently.

The initialization functions are:

Proposed cleanup I propose to define what these functions do for a physics solver in SolverBase and have a final implementation there and then have physics solver specific functions to initialize things. Something like

void initialize_postMeshGeneration override final;
void initializePreSubGroups  override final;
void initializePostSubGroups  override final;
void initializePostInitialConditionsPreSubGroups  override final;
void initializePostInitialConditionsPostSubGroups  override final
{
  initializePrimaryVariables();
  initializeConstitutiveModels();
}

virtual void  initializePrimaryVariables();
virtual void  initializeConstitutiveModels();

This way we could avoid patterns which seem to be a bit arbitrary like the fact that this step is performed here https://github.com/GEOSX/GEOSX/blob/00dd40335a8b3dae72adb5ebc4e7b181e7b62051/src/coreComponents/physicsSolvers/fluidFlow/SinglePhaseBase.cpp#L231 but other initializations are performed here https://github.com/GEOSX/GEOSX/blob/00dd40335a8b3dae72adb5ebc4e7b181e7b62051/src/coreComponents/physicsSolvers/fluidFlow/SinglePhaseBase.cpp#L339 instead. Similarly, the linear algebra parameters for the physics solvers are not always set in the same function. These small inconsistencies make it hard when writing a coupled solver because the order in which things occur is not always clear.

I guess we could also introduce a new intialization step, less generic than the one provided by Group, which, for example, explicitly takes DomainPartition as an input.

TotoGaz commented 1 year ago

In your functions, every time there is a SubGroups, are you able to tell if there's an effective type for those SubGroups? If so, you may rename those function into more comprehensive functions.

Meanwhile, what are the ideas behind all these initializations? I image there's something to initialize the fields for the simulations start, maybe the boundary conditions, etc... If you manage to build big initialization families, you can surely built on this?

CusiniM commented 1 year ago

In your functions, every time there is a SubGroups, are you able to tell if there's an effective type for those SubGroups? If so, you may rename those function into more comprehensive functions.

Not sure I follow here. These functions are defined in Group so they don't really have access to anything apart from what's owned by the object itself. However, we often do things like:

DomainPartition & domain = this->getGroupByPath< DomainPartition >( "/Problem/domain" ); which then gives us access to everything. We could at least centralize this call for all solvers so that we know where it happens.

Meanwhile, what are the ideas behind all these initializations? I image there's something to initialize the fields for the simulations start, maybe the boundary conditions, etc... If you manage to build big initialization families, you can surely built on this?

yeah, I think this is a good idea. I can think of a few things:

TotoGaz commented 1 year ago

In your functions, every time there is a SubGroups, are you able to tell if there's an effective type for those SubGroups? If so, you may rename those function into more comprehensive functions.

Not sure I follow here. These functions are defined in Group so they don't really have access to anything apart from what's owned by the object itself.

What I mean is that those functions surely do not have the same meaning depending on the context of the class deriving from Group. I will take another example, the Group::numSubGroups() member function.

In other words, different contexts, different meanings. So these initialize*Groups may have a specific meaning when it comes to Solvers than when it comes to e.g. ObjectManagerBase instances. My comment was about trying to extract the "deep/underlying" SolverBase meaning of those initialize*Groups members.

However, we often do things like:

DomainPartition & domain = this->getGroupByPath< DomainPartition >( "/Problem/domain" ); which then gives us access to everything. We could at least centralize this call for all solvers so that we know where it happens.

Note that we already have one in ProblemManager, but we indeed could have some wrapper like

DomainPartition & getDomain() {
  return this->getGroupByPath< DomainPartition >( "/Problem/domain" );  // TODO use constants!
}

Where would you put it? I'd guess that the common factor for all the calls would be Group 🤷 And as such you make Group depend on DomainPartition while DomainPartition already depends on Group through direct inheritance? So you create a direct dependency cycle. C++ can surely trick the build system into something working using forward declarations, but this may not be great design nevertheless.

Honestly, this is the kind of question that are surprisingly tricky to solve.

I can think of a few things:

  • initialize variables, including copying the current state into the state at time n (e.g, p_n = p )
  • initialize constitutive models (perform initial calculations)
  • initialize state of the system, i.e., solve for equilibrium

Currently, can we investigate in the code what's being done? Then we have the exact full list of items.

CusiniM commented 1 year ago

What I mean is that those functions surely do not have the same meaning depending on the context of the class deriving from Group. I will take another example, the Group::numSubGroups() member function.

* `ElementRegionManager::numSubGroups()` returns the number of regions.

* `FluxApproximationBase::numSubGroups()` returns the number of finite volume stencils. Does it make sense or is it useful? I do not know... But we can still call the function.

In other words, different contexts, different meanings. So these initialize*Groups may have a specific meaning when it comes to Solvers than when it comes to e.g. ObjectManagerBase instances. My comment was about trying to extract the "deep/underlying" SolverBase meaning of those initialize*Groups members.

However, we often do things like: DomainPartition & domain = this->getGroupByPath< DomainPartition >( "/Problem/domain" ); which then gives us access to everything. We could at least centralize this call for all solvers so that we know where it happens.

Note that we already have one in ProblemManager, but we indeed could have some wrapper like

DomainPartition & getDomain() {
  return this->getGroupByPath< DomainPartition >( "/Problem/domain" );  // TODO use constants!
}

Where would you put it? I'd guess that the common factor for all the calls would be Group 🤷 And as such you make Group depend on DomainPartition while DomainPartition already depends on Group through direct inheritance? So you create a direct dependency cycle. C++ can surely trick the build system into something working using forward declarations, but this may not be great design nevertheless.

No I would not touch Group. For the specific case of physics solvers I was thinking that we could narrow down the meaning of these initialization functions. I was basically suggesting to only use this generic interface inherited from Group in the base class (SolverBase) and try to enforce it by defining final override in SolverBase (obviously not bulletproof).

Then we would have physics solvers specific initialization functions defined as virtual methods in SolverBase . Those functions could then be defined by each physics solver individually and they would be things like: virtual void initializePrimaryVariables(DomainPartition & domain);, virtual void initializeConstitutiveModels(DomainPartition & domain);, virtual void computeInitialState(DomainPartition & domain);.

Then, we could get the DomainPartition in SolverBase. Obviously this does stop someone from just doing it somewhere else in the code nor it prevents ppl from using generic interface functions but it should at least create a virtuous pattern.