CF Merger #3: Compositional Flow models #1236
Conversation
…sions and make equilibrium optional.
…callables on domains.
…ondaryExpression.
…terate index, and all grid types.
…ities for clarity.
…LEE and secondary equations.
… SecondaryExpression.
…ive expressions for models without equilibrium equations.
…on in CF now properlt sized.
There was a problem hiding this comment.
@vlipovac: This is a partial review only, as I ran out of energy in the middle of compositional_flow.py and will have to look at that file again. Also, I have not had a look at the tests or the tutorial, but will try to do so over the weekend. Still, there should be plenty of remarks to entertain you, if you should be inclined to have a look. Some are concrete, others vaguer, let me know if you want to discuss. Though I have tried to be thorough, I cannot promise I will not find new remarks when having a second look.
I have also tagged @IvarStefansson at comments that will benefit from his opinion.
Also, I have pushed some changes, mainly documentation, that may have broken something. Sorry.
src/porepy/__init__.py
Outdated
| @@ -196,23 +198,24 @@ | |||
| ReferenceVariableValues, | |||
| ) | |||
| from porepy.compositional.base import Component, Phase, Fluid | |||
| from porepy.compositional.compositional_mixins import CompositionalVariables, FluidMixin | |||
| from porepy.compositional.compositional_mixins import * | |||
There was a problem hiding this comment.
Two comments:
- While I see the argument for the * import, and appreciate the definition of all in compositional_mixins, this will be the first * import in this top-level file. I am not convinced this is something we want to open up for.
- I am worried about the namespace of the top-level pp becoming cluttered. Despite much thinking, we have never managed to design an import system that gives a reasonable reflection of the hierarchy of packages. My instinct is to introduce some subpackage, say cf (compositional flow) and to imports of the type pp.cf.{class/method}. @IvarStefansson: Your opinion will be needed here - can we find a solution for the compositional functionality that does not require that we decide what to do with the whole of porepy?
There was a problem hiding this comment.
I don't feel I know the impact/usage of this, so I'm not sure my opinion is very well founded. Would an option to include only a subset to top-level and in files using compositional extensively simply import compositional explicitly?
There was a problem hiding this comment.
Reverted this change to import only the Variables and fluid mixin classes.
Changed the usage of the other function imported by the star-import to
import porepy.compositional as compositional
compositional.function()
throughout porepy.
There was a problem hiding this comment.
I guess the reason for keeping the variables and fluidmixin is that these will show up in all models, since all models in a sense are compositional? If correct, please add a short comment to this effect, so that I don't get confused about what is so special about these classes whenever I look at this line.
There was a problem hiding this comment.
All models have the fluid mixin, that is correct. But only truly multiphase multicomponent models require the CompositionalVariables. Do you want only the FluidMixin included on the top level?
| @@ -61,9 +66,88 @@ def _get_surrogate_factory_as_property( | |||
| ) | |||
|
|
|||
|
|
|||
| # NOTE The following methods are outsourced for compatibility reasons. | |||
There was a problem hiding this comment.
Similar functionality already exists as methods of the models proper (example: is_linear or whatever - this is not strictly a parameter lookup as is the case below, but in a wider sense it answers an enquiry about the model properties). Considering that all models are considered multiphase multicomponent, with multi=1 by default, is it not more consistent to make these model methods (or properties ?) instead? This will also counteract the pp namespace becoming overcrowded.
@IvarStefansson: You will have an opinion on this one.
There was a problem hiding this comment.
Yes, I believe it makes sense to try to unify. The reason for making these methods was to allow the default implementation to depend on the model class (incompressible flow is linear, thermoporomechanics nonlinear). However, if there are specific reasons to do otherwise, I'm open to discussing.
There was a problem hiding this comment.
I am open to put this into the base SolutionStrategy like the methods Eirik mentioned.
Originally I was hesitant to do that because two of them are only relevant for models with flash calculations.
There was a problem hiding this comment.
Let's move them into the solution strategy, then.
There was a problem hiding this comment.
I moved the inquires about eliminated reference phase and component to the base SolutionStrategy, because it has a general fluid now and it is therefore compatible with all models.
I moved the fractional flow inquire to compositional_flow, because it is only used there, and I removed imports in packages on any level since not required.
The inquires about the equilibrium type I will leave for now because they are required to determine the DOFs for CF.
Added a note to revisit their placing once the flash and CFLE are ready to be merged.
If this is fine with you, please resolve.
| import porepy as pp | ||
|
|
||
|
|
||
| class InitialConditionMixin(pp.PorePyModel): |
There was a problem hiding this comment.
@IvarStefansson Can you have a look at this concept, and its use in existing models (search for inheritance from this class). A bit of background: Compositional flow introduces secondary variables to a much larger extent than what we have had before, and this implies more ellaborate initialization procedures, hence the need to work more in a more structured manner with this. There is a hint of this in the example starting 10 lines below from here.
Mainly documentation
There was a problem hiding this comment.
Hi @vlipovac. I have been through the entire PR now, more or less thoroughly. Main comments and thoughts:
- This is highly impressive, both in scope and the overall quality of the produced code. Well done!
- While I have given a lot of comments and suggestions, many of these should be relatively easy to fix.
- I have identified a few suggestions that are in effect lager choices of design (e.g., initial conditions). Here I have tried to take a critical but constructive view, but also asked Ivar to have a second look.
- A few of the comments probably reflects my confusion regarding the formulation being used etc. Please bear with me, but when relevant also take it as feedback on the level of documentation needed. This applies in particular to everything relating to boundary conditions.
Processes ahead: I suggest you have a look when you have the time to do so, and we then decide how to move forward. I have asked Ivar to weigh in on some points, and he will do so in due course. Again considering the scope and complexity, there will likely also be a secondary review of sorts, by me and/or Ivar, but hopefully the two of us can manage to narrow down the scope of questions before that.
| ): | ||
| """Combines mass and momentum balance equations.""" | ||
|
|
||
| def set_equations(self): |
There was a problem hiding this comment.
@IvarStefansson: This is an example of what I mentioned with use of super, MRO etc. Could you have a look?
There was a problem hiding this comment.
I changed the order here, because there was some issue in the tests.
One of them had a hardcoded assumption on the order of equations, which was changed in this PR which uses the super framework fully.
|
|
||
| return z_0 + z_inflow | ||
|
|
||
| def diffused_tracer_fraction(self, sd: pp.Grid, t: float) -> np.ndarray: |
| @@ -61,9 +66,88 @@ def _get_surrogate_factory_as_property( | |||
| ) | |||
|
|
|||
|
|
|||
| # NOTE The following methods are outsourced for compatibility reasons. | |||
There was a problem hiding this comment.
Yes, I believe it makes sense to try to unify. The reason for making these methods was to allow the default implementation to depend on the model class (incompressible flow is linear, thermoporomechanics nonlinear). However, if there are specific reasons to do otherwise, I'm open to discussing.
| from porepy.numerics.ad.equation_system import GridEntity | ||
|
|
||
|
|
||
| class EquationMixin(pp.PorePyModel): |
There was a problem hiding this comment.
Yes, I think the abstraction is fine. But is "mixin" necessary in the name of this class (and possibly others)?
| ) | ||
|
|
||
| def initial_pressure(self, sd: pp.Grid) -> np.ndarray: | ||
| """ |
There was a problem hiding this comment.
Please add a descriptive sentence.
| @@ -835,3 +888,85 @@ class PorePyModel( | |||
| runtime, since it is not an abstract base class. | |||
|
|
|||
| """ | |||
|
|
|||
| class CompositionalFlowModelProtocol(PorePyModel, Protocol): | |||
There was a problem hiding this comment.
I was less involved towards the end of the work on the protocols. I don't immediately see strong reasons to avoid this, but wonder whether it could be better to keep them separate, i.e., not inherit from PorePyModel. Also, let's consult @Yuriyzabegaev
|
@vlipovac I have resolved a lot of conversations, and also marked some as ready for you to resolve whenever you feel done. |
PR for the next step in including the branch pmgbergen/porepy/tree/composite-flow
This PR contains the compositional flow model, related mixins, as well as a 2-component tracer flow example with tutorial.
Proposed changes
The file compositional_flow implements the new model and is the main contribution of this PR, and I think it's structure is self-explanatory as to which model parts are introduced and required for a general CF model.
The file initial_condition introduces a new layer for easier definition of IC in a multi-physics setting with multiple variables. The layer is added to existing models.
A new type of equation is added to abstract_equations, called
LocalElimination. It allows to eliminate some variable using a function and some declared dependencies (other variables). The framework handles both IC and BC for the eliminated variable and theSurrogateFactoryis used in combination with the function argument to obtain an AD representation of this local elimination.The file tracer_flow contains a single-phase, 2-component flow model (tracer flow), which is showcasing how to set up a simple CF model. A new tutorial is accompanying this model and explains the steps in more detail.
The sub-package porepy/examples/geothermal_flow contains the example models provided by Omar and Michael. Due to my parental leave I am not aware how much you have discussed about how to include them. I put them in this draft since they serve as excellent examples on what is required to set up a CF model.
NOTE 28-11-2024: After discussion in person, the geothermal_flow will be removed from this PR.
Testing
@mikeljordan proposed a test using a linear tracer flow model (single-phase, 2-component) in combination with an analytical solution. The respective test can be found here here
A second tests extends the first test artificially to include additional 2 phases with fixed saturations of 1/3 and identical IC, BC, densities and viscosities.
The model per se has no meaning, but it tests the full machinery of multi-phase, multi-component flow, including having phase properties as surrogate operators and local equation framework.
Additionally, the two-variable energy balance is introduced to check whether the model is able to reflect isothermal conditions, adding an additional verification layer.
Unification of notion of
fluidThe CF models have a generalized notion of fluid allowing multiple phases and components, currently coded as a model attribute
fluid_mixture, based on the recently introduced extensionporepy.compositional.The (legacy) models have a data structure for fluid constants, on which various constitutive laws are built.
After talking with @keileg , the idea is for me to go ahead and propose a way to wrap the existing fluid constants into a generic 1-phase-1-component mixture, which then hopefully will be integrate into the existing models.
It is also worthwhile to consider extending the model attribute
fluidto be a generalFluidMixture, in order to not have two various conceptsNOTE 31-10-2024: Unification is on the way in #1244
NOTE 5-11-2024: Notion of fluid is unified and scripts were adapted correspondingly (see #1252 )
Code unification and recycling with existing
fluid_mass_balanceandenergy_balanceThere is vast space for code recycling, since the CF model currently does many things on its own, which were to a less general degree implemented in the existing fluid and energy models. My sense is that with some minor refactoring, the existing models can be included in the CF.
This point is mainly connected to the notion of
fluid, and how to access relevant members such as fluid density, enthalpy etc.But also some thoughts on the advective flux are required (f.e. a general declaration of the non-linear weight, which is in different forms implemented in the existing
energy_balanceandfluid_mass_balance).If I am not mistaken, @IvarStefansson this is most relevant for you.
NOTE 31-10-2024: Unification is planned. First, minor steps were taken in #1252 (see #1252 (comment)), more thought on a general
AdvectionEquationor something of that sorts is required.NOTE 28-11-2024:
MassBalanceEquationsandEnergyBalanceEquationsfor them to be usable in the CF setting as pressure and total energy balance equations respectively. Inconsistencies remaining in the discretization of hyperbolic/advective parts.super()functionality is now fully utilized in coupled-physics models with multiple mixins representing equations and variables.AdvectionEquationis not clear at this point, mostly due to unclear handling of BC for elliptic and hyperbolic discretizations (MPFA & upwinding).NOTE 09-12-2024:
A unification of equations was done by refactoring some inner workings of existing mass and energy balance. They serve now as balance equations for total mass and energy.
Types of changes
Checklist
pytestwas run with the--run-skippedflag.