Initialization - #1686
Conversation
…nce (not introduced yet).
jwboth
left a comment
There was a problem hiding this comment.
@IvarStefansson @keileg I tried highlighting the pieces in the code that I suspect to require a critical look. Hope it helps for reviewing the suggested concepts.
| has_momentum_balance_reference_state: bool = False | ||
| """Flag whether reference states have been defined.""" |
There was a problem hiding this comment.
The changes in this class are representative for changes in other core model classes. Be critical about:
- explicit introduction of essentially three versions of a variable (the absolute one derives from the others by addition)
- the concept behind updating and shifting values including the flag above - it feels like one should have maybe the possibility to do this in a more global way and not model by model. Is it possible to utilize some decorators or collect variables in respective containers? Bit like with
previous_timestep(). Argument against a global approach would possibly be memory. One could also argue for (like @IvarStefansson did yesterday) that we could ignore reference states in the remaining physics (pressure, temperature etc) since displacements are the only singular object and require correct treatment in terms of modelling, while the remaining once may instead only impact the numerical properties like conditioning. - the imposed DataSavingMixin that always exports all three variants for the variables (maybe too much?). Reference variables should for instance also not change over time and could be handled as constants (see the not so much used functionality of the Exporter)
There was a problem hiding this comment.
I agree that there seems to be unnecessary amount of boilerplate.
| class MechanicalAperture: | ||
| """Mechanical aperture of fractures wrt reference configuration.""" | ||
|
|
||
| normal_component: Callable[[list[pp.Grid]], pp.ad.Operator] | ||
| """Operator giving the normal component of a vector on fractures.""" | ||
|
|
||
| displacement_jump: Callable[[list[pp.Grid]], pp.ad.Operator] | ||
| """Displacement jump on fractures wrt reference .""" |
There was a problem hiding this comment.
Be critical about the handling of the difference apertures and the now explicit use of relative displacements.
| NOTE: The implementation of the mechanical stress below is in absolute terms. | ||
| The stress could be formulated wrt a reference configuration, in which case | ||
| a reference (background) stress would be required, only depending on the reference | ||
| displacement. Under the assumption of linearity, the summation of the reference and | ||
| the linear mechanical stress below cancels the reference contributions. For | ||
| simplicity, background stress is not supported in the current implementation. | ||
|
|
There was a problem hiding this comment.
Should one think more about including explicit background stress measurements or possibly a homogeneous background stress and then formulate the mechanical stress in terms of relative quantities?
| @@ -4549,7 +4619,7 @@ def porosity(self, subdomains: list[pp.Grid]) -> pp.ad.Operator: | |||
|
|
|||
|
|
|||
| class PoroMechanicsPorosity(pp.PorePyModel): | |||
There was a problem hiding this comment.
Note that I did not cover all constitutive laws, e.g. code related to BartonBandis , damage etc.
What should be the right strategy? Coverage or sanity checks throwing an error/warning if reference states are used e.g. for unsupported constitutive laws or models.
| def relative_contact_traction(self, subdomains: list[pp.Grid]) -> pp.ad.Operator: | ||
| """Fracture contact traction increment [-]. |
There was a problem hiding this comment.
Be critical here: Do we really need reference and relative versions of all variables? There is no constitutive law that will benefit from contact traction changes, or? Same would apply for saturation-type variables in multiphase flow settings. If there should really be an impact on the linear/nonlinear conditioning of the problem by solving for increments, one should maybe consider solving for increments wrt the previous time step?
| """ | ||
| return Scalar(0.0, "reference_mechanical_aperture") | ||
|
|
||
| def mechanical_aperture(self, subdomains: list[pp.Grid]) -> pp.ad.Operator: |
There was a problem hiding this comment.
Consider if this makes reference fracture gap redundant/is already covered by that.
|
|
||
|
|
||
| class DisplacementJumpAperture(DimensionReduction): | ||
| class DisplacementJumpAperture(DimensionReduction, MechanicalAperture): |
There was a problem hiding this comment.
We need to be very careful about what this does to the interpretation of aperture (hydraulic vs mechanical)
| """See :class:`VariablesSinglePhaseFlow`.""" | ||
|
|
||
| interface_darcy_flux: Callable[ | ||
| relative_interface_darcy_flux: Callable[ |
There was a problem hiding this comment.
Do we need relative fluxes?
| has_momentum_balance_reference_state: bool = False | ||
| """Flag whether reference states have been defined.""" |
There was a problem hiding this comment.
I agree that there seems to be unnecessary amount of boilerplate.
|
@jwboth Have I understood correctly that this PR is being replaced by the series of newer PRs on the same topic? |
Three-fold extension:
The latter is defined for initialization of quasi-static initial conditions as well as reference states.
Types of changes
What types of changes does this PR introduce to PorePy?
Put an
xin the boxes that apply.Checklist
Put an
xin the boxes that apply or explain briefly why the box is not relevant.pytestwas run with the--run-skippedflag.