-
Notifications
You must be signed in to change notification settings - Fork 88
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
Extending fluid concept to fit CF & Standard formulations #1244
base: develop
Are you sure you want to change the base?
Conversation
…d, instead of FluidConstants.
…d deleting chem_species.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Here is a self-review with the essential changes.
All other changes are essentially due to the change in the interface to fluid properties:
model.fluid_density()
-> model.fluid.density
and similar.
Also, the access to FluidConstants and SolidConstants is not done by calling anymore, thanks to the data class framework.
# By providing default values to the fields, not every constant is required. The user | ||
# is expected to be aware which physics are used in the model. | ||
@dataclass(frozen=True, kw_only=True) | ||
class MaterialConstants: |
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.
Material constants are reworked to be a data class structure, frozen and kw-only.
This ensures that constants are indeed constants (not changing during simulation) and the instantiation is made easier with kw-only, enabling some mechanisms to control the user input (valid and invalid constants)
Post-init processing was added for automatic conversion, making separate methods for constants with some physical unit redundant (the conversion was done in those methods before).
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.
Does 'some physical unit' mean quantities that are not dimensionless?
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.
Yes. The original implementation performed a conversion when the constant was called, IF it had a physical dimension. Otherwise it just returned the constant.
Now the post init processing takes care of the conversion, IF a dimension is given in SI_units, and we can define constants simply using the dataclass mechanism.
@@ -339,32 +356,48 @@ def reset_state_from_file(self) -> None: | |||
def set_materials(self): |
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 method is reworked to create the model attribute fluid
as a general Fluid
, including phase and component context.
The mixed-in method create_fluid
is provided by the porepy.compositional.FluidMixin
# NOTE this is critical in the case where properties depend on some fractions. | ||
# The callables for those are dynamically created during create_variables, as opposed | ||
# to e.g., pressure or temperature. | ||
self.assign_thermodynamic_properties_to_phases() |
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 step was added to the set-up.
It is critical that the Fluid is created in set_materials
, before the variables, since the dynamic number of variables depends on the fluid definition.
The step assign_thermodynamic_properties_to_phases
requires the variables to be already created, since fluid properties are in general secondary terms which depend on some variable (keyword SurrogateFactory)
@@ -0,0 +1,375 @@ | |||
""""Module containing some constant heuristic fluid property implemenations. |
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 file takes fluid-related heuristics out of the constitutive_laws.py
file.
Some methods are reworked to fit the general fluid concept, where properties of phases are dynamically created based on the number of modelled phases.
@@ -1095,25 +1129,37 @@ def create_mixture(self) -> None: | |||
|
|||
self.set_components_in_phases(components, phases) | |||
|
|||
self.fluid_mixture = FluidMixture(components, phases) | |||
self.fluid = Fluid(components, phases) | |||
|
|||
def get_components(self) -> list[Component]: |
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 method has now a default-behavior, creating a one-component fluid based on the fluid constants passed by the user. Important to mention is that the simulation set-up does not change for the standard 1-phase, 1-component case.
Users still use and pass a FluidConstants instance in params.
raise CompositionalModellingError( | ||
"Call to mixin method. Configure phases by overriding this method." | ||
) | ||
return [(AbstractEoS(components), PhysicalState.liquid, "liquid")] |
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.
The default behavior of get_phase_configuration was also changed to model a 1-phase fluid with a liquid-like phase.
The AbstractEoS was modified for this purpose, not being abstract anymore but playing the role of the "mathematician's" EoS, i.e. a dummy doing nothing for models using heuristic fluid properties (constitutive_laws.py)
self.fluid_mixture.density = self.fluid_density | ||
self.fluid_mixture.specific_volume = self.fluid_specific_volume | ||
self.fluid_mixture.specific_enthalpy = self.fluid_specific_enthalpy | ||
|
||
def dependencies_of_phase_properties( |
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 is a bit tricky.
I had to change the default behavior of this, because of the mechanics-only models, who inherit from SolutionStrategy.
Problems include the missing of the classical variable mixins like pressure and temperature.
The default return value is now an empty list, leading to fluid properties which raise an error if they were not overridden by some constitutive law.
It makes sense, but may be hard to digest for people digging deeper into the code.
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 this makes sense, but it should be properly docemunted here and at the point where the error is raised
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.
Improved documentation, here and at the default implementation of fluid properties in this class.
@@ -957,3 +924,137 @@ def reference_component(self) -> Component: | |||
"""Returns the reference component as designated by | |||
:meth:`reference_component_index`.""" | |||
return self._components[self.reference_component_index] | |||
|
|||
def density(self, domains: pp.SubdomainsOrBoundaries) -> pp.ad.Operator: |
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.
The class Fluid
(formerly FluidMixture) is now the new interface for other model mixin classes to access fluid properties.
It implements the fluid properties required by the PDE classes, while the framework handles the assignment of heuristic functions to the (single) phase properties under the hood.
@@ -354,6 +352,12 @@ def test_tested_vs_testable_methods_single_phase_flow( | |||
("well_fluid_flux", 0, 2), # use dim_restriction=2 to ignore well flux | |||
("well_flux_equation", 0, 2), # use dim_restriction=2 to ignore well equation | |||
("well_radius", 0.1, None), | |||
# Testing of methods with deeper namespace |
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.
The testing of the fluid mass balance was modified and adapted to the new interface for fluid properties.
The test was extended to cover the standard model implemented (1 phase 1 component).
Thanks @vlipovac. I'll get started on this later today. |
@@ -304,6 +300,23 @@ def interface_energy_flux(self, interfaces: list[pp.MortarGrid]) -> pp.ad.Operat | |||
flux.set_name("interface_energy_flux") | |||
return flux | |||
|
|||
def mobility_rho_h(self, domains: pp.SubdomainsOrBoundaries) -> pp.ad.Operator: |
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 introduced this, analogous to the ominous mob_rho
in fluid mass balance.
It's not perfect, but it eliminates the multiple occurrences of rho * h / mu
throughout the class, including a scoped definition of this function at some point.
This is also 1 step towards generalizing Balance equations in terms of advection and diffusion.
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.
Your idea is to do the generalization when introducing the compositional flow model class?
…d, instead of FluidConstants.
…d deleting chem_species.
Co-authored-by: Eirik Keilegavlen <[email protected]>
Added changes as per #1247 I rebased the branch onto the recent version of develop which includes Protocols. |
|
||
Note that the protocol framework is accessed by static type checkes only! | ||
|
||
Warning: |
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.
Adding this warning to reflect the insights gained with @Yuriyzabegaev
The runtime PorePyModel
class does not inherit from Protocol
anymore.
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.
Some minor comments before I had to run for the weekend. I think most of my concerns are taken care of, I'll try to resolve comments when I have the time.
Navigating the changes in difficult due to the protocol changes being included. I suggest you consider to formally close this PR (after we have an overview of which comments have been taken care of), and then open a new one before Ivar has a look - that should make things easier for him. does this make sense?
def save_data_time_step(self) -> None: | ||
"""Export the model state at a given time step, and log time. | ||
The options for exporting times are: | ||
* None: All time steps are exported |
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 we want to delete the docstrings from these methods, even though they are also present in the protocol. Do you agree @Yuriyzabegaev?
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 applies to several methods in this file.
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.
Yes, the current convention is to duplicate docstrings. This is something we can mention on the next user meeting.
mobility_keyword: str | ||
"""Keyword for the discretization of the mobility. Normally provided by a mixin of | ||
instance :class:`~porepy.models.SolutionStrategy`. | ||
|
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.
Let's try not to introduce new todos unless they are strictly necessary.
Closes #1240
Closes #1247
Proposed changes
Reworking the concept of
model.fluid
to be a general fluid with phases and components, instead of just a data collection of constants.Breaking change with a multiple minor changes throughout the packages. The essential changes are summarized in a self-review.
porepy.compositional.ChemicalSpecies
was merged intoFluidConstants
, including data class functionality. This led to some changes forSolidConstants
as well for inheritance reasons.Types of changes
Checklist
pytest
was run with the--run-skipped
flag.