Extending fluid concept to fit CF & Standard formulations #1244

vlipovac · 2024-10-21T14:54:34Z

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 into FluidConstants, including data class functionality. This led to some changes for SolidConstants as well for inheritance reasons.

Types of changes

Minor change (e.g., dependency bumps, broken links).
Bugfix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).
Testing (contribution related to testing of existing or new functionality).
Documentation (contribution related to adding, improving, or fixing documentation).
Maintenance (e.g., improve logic and performance, remove obsolete code).
Other:

Checklist

The documentation is up-to-date.
Static typing is included in the update.
This PR does not duplicate existing functionality.
The update is covered by the test suite (including tests added in the PR).
pytest was run with the --run-skipped flag.

…amework.

…d, instead of FluidConstants.

…d deleting chem_species.

…nts.

…convention.

review-notebook-app · 2024-10-21T14:54:41Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

vlipovac

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.

vlipovac · 2024-10-21T14:57:17Z

src/porepy/compositional/materials.py

+# 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:


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).

Does 'some physical unit' mean quantities that are not dimensionless?

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.

vlipovac · 2024-10-21T14:59:01Z

src/porepy/models/solution_strategy.py

@@ -339,32 +356,48 @@ def reset_state_from_file(self) -> None:
    def set_materials(self):


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

vlipovac · 2024-10-21T15:00:38Z

src/porepy/models/solution_strategy.py

+        # 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()


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)

vlipovac · 2024-10-21T15:01:50Z

src/porepy/models/fluid_property_library.py

@@ -0,0 +1,375 @@
+""""Module containing some constant heuristic fluid property implemenations.


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.

vlipovac · 2024-10-21T15:04:10Z

src/porepy/compositional/compositional_mixins.py

@@ -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]:


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.

vlipovac · 2024-10-21T15:06:02Z

src/porepy/compositional/compositional_mixins.py

-        raise CompositionalModellingError(
-            "Call to mixin method. Configure phases by overriding this method."
-        )
+        return [(AbstractEoS(components), PhysicalState.liquid, "liquid")]


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)

vlipovac · 2024-10-21T15:08:13Z

src/porepy/compositional/compositional_mixins.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(


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.

I think this makes sense, but it should be properly docemunted here and at the point where the error is raised

Improved documentation, here and at the default implementation of fluid properties in this class.

vlipovac · 2024-10-21T15:10:27Z

src/porepy/compositional/base.py

@@ -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:


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.

vlipovac · 2024-10-21T15:11:41Z

tests/models/test_fluid_mass_balance.py

@@ -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


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).

keileg · 2024-10-22T08:05:42Z

Thanks @vlipovac. I'll get started on this later today.

vlipovac · 2024-10-22T11:03:21Z

src/porepy/models/energy_balance.py

@@ -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:


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.

Your idea is to do the generalization when introducing the compositional flow model class?

…d, instead of FluidConstants.

…d deleting chem_species.

…nts.

…convention.

Co-authored-by: Eirik Keilegavlen <[email protected]>

src/porepy/models/protocol.py

vlipovac · 2024-10-25T12:05:07Z

Added changes as per #1247

I rebased the branch onto the recent version of develop which includes Protocols.
A models.protocol.FluidProtocol is included now.
I think we can have a second round of review @keileg @IvarStefansson .
Also, most of the remarks made by Eirik should be fixed now.

vlipovac · 2024-10-25T13:57:55Z

src/porepy/models/protocol.py

+
+Note that the protocol framework is accessed by static type checkes only!
+
+Warning:


Adding this warning to reflect the insights gained with @Yuriyzabegaev
The runtime PorePyModel class does not inherit from Protocol anymore.

keileg

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?

keileg · 2024-10-25T14:07:25Z

src/porepy/viz/data_saving_model_mixin.py

    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


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?

This applies to several methods in this file.

Yes, the current convention is to duplicate docstrings. This is something we can mention on the next user meeting.

keileg · 2024-10-25T14:11:01Z

src/porepy/models/fluid_property_library.py

+    mobility_keyword: str
+    """Keyword for the discretization of the mobility. Normally provided by a mixin of
+    instance :class:`~porepy.models.SolutionStrategy`.
+


Let's try not to introduce new todos unless they are strictly necessary.

vlipovac added 19 commits October 17, 2024 08:57

MOD: Renaming classes and methods for compatibility with modelling fr…

3f80872

…amework.

MOD: Material constants are now data classes replacing chemical species.

8fffb9b

MOD: Make hashable dict for material constants ordered.

5b9e0af

MIN: Moving material constants to porepy.compositional.materials

014fbf5

TEST: Adapting tests to new signature of material constants.

559a4b3

MOD: changing solution strategy to create a 1-phase, 1-component flui…

87437a8

…d, instead of FluidConstants.

MIN: Import and instantiation issues after change in material constants

3fec45b

MOD: Adapting usage of SolidConstants throughout package.

4b93858

MOD: moving all functionality from chem_species.py to materials.py an…

0fce4f1

…d deleting chem_species.

MOD: Adapted usage of FluidConstants to recent changes.

32a768b

Merge branch 'develop' into fluid_concep

3dc1964

MIN: Removing some unused mixin annotations.

349d5d0

MOD: Adapting some remaining, untyped usage of fluid and solid consta…

c07a4d8

…nts.

MOD: Refactor fluid constitutive laws to general fluid.

c72012d

MIN: Changing argument names for fluid property function to match pp …

e2c6c7f

…convention.

MIN: Shorten namespace of some objects in pp.compositional to pp.*

8bd3179

MOD: Adding FluidMixin to base models.

7c91748

TEST: All tests pass now.

18a035e

STY: mypy, flake8, isort, black

3ec6590

vlipovac added enhancement New feature or request. priority - high Issue/PR of high priority. labels Oct 21, 2024

vlipovac requested review from keileg and IvarStefansson October 21, 2024 14:54

vlipovac self-assigned this Oct 21, 2024

vlipovac requested a review from jwboth as a code owner October 21, 2024 14:54

vlipovac commented Oct 21, 2024

View reviewed changes

keileg self-assigned this Oct 22, 2024

vlipovac commented Oct 22, 2024

View reviewed changes

vlipovac and others added 23 commits October 25, 2024 09:54

TEST: Adapting tests to new signature of material constants.

ccc35e1

MOD: changing solution strategy to create a 1-phase, 1-component flui…

9855184

…d, instead of FluidConstants.

MIN: Import and instantiation issues after change in material constants

a0c0721

MOD: Adapting usage of SolidConstants throughout package.

fc900d9

MOD: moving all functionality from chem_species.py to materials.py an…

5ad1ef1

…d deleting chem_species.

MOD: Adapted usage of FluidConstants to recent changes.

a182cbe

MOD: Adapting some remaining, untyped usage of fluid and solid consta…

bd8aa16

…nts.

MOD: Refactor fluid constitutive laws to general fluid.

9f2c380

MIN: Changing argument names for fluid property function to match pp …

ff53f37

…convention.

MIN: Shorten namespace of some objects in pp.compositional to pp.*

95a08b7

MOD: Adding FluidMixin to base models.

ca91c68

TEST: All tests pass now.

38a395d

STY: mypy, flake8, isort, black

d0c115c

Apply suggestions from code review

eaa2c77

Co-authored-by: Eirik Keilegavlen <[email protected]>

Applying minor changes from review.

94bf9c6

MOD: transferring conver_units to Units class (issue 1247)

7fe5a2a

ENH: Introducing the FluidProtocol into the base PorePyModel

5c043d5

FIX: Lost changes after rebase onto develop (with protocols).

b0e7117

Mergde 'origin/fluid_concept' into fluid_concept.

4460b2f

MIN: Docs of fluid protocol.

b61a416

MIN: Docs in fluid protocol

5f62371

STY: flake8, black

b6dc203

TEST: All tests pass now.

495df0e

vlipovac commented Oct 25, 2024

View reviewed changes

src/porepy/models/protocol.py Outdated Show resolved Hide resolved

vlipovac added 3 commits October 25, 2024 14:14

testing

8d6ec7c

FIX: Inconsistent assignment of constructor in porepy models.

da986bf

MIN: Improving docs and clean-up.

a2a6c4f

vlipovac commented Oct 25, 2024

View reviewed changes

keileg reviewed Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extending fluid concept to fit CF & Standard formulations #1244

Extending fluid concept to fit CF & Standard formulations #1244

vlipovac commented Oct 21, 2024 •

edited

Loading

review-notebook-app bot commented Oct 21, 2024

vlipovac left a comment

vlipovac Oct 21, 2024

keileg Oct 22, 2024

vlipovac Oct 23, 2024

vlipovac Oct 21, 2024

vlipovac Oct 21, 2024

vlipovac Oct 21, 2024

vlipovac Oct 21, 2024

vlipovac Oct 21, 2024

vlipovac Oct 21, 2024 •

edited

Loading

keileg Oct 23, 2024

vlipovac Oct 24, 2024

vlipovac Oct 21, 2024

vlipovac Oct 21, 2024

keileg commented Oct 22, 2024

vlipovac Oct 22, 2024

keileg Oct 23, 2024

vlipovac commented Oct 25, 2024

vlipovac Oct 25, 2024

keileg left a comment

keileg Oct 25, 2024

keileg Oct 25, 2024

Yuriyzabegaev Oct 25, 2024

keileg Oct 25, 2024

		@@ -339,32 +356,48 @@ def reset_state_from_file(self) -> None:
		def set_materials(self):

		@@ -0,0 +1,375 @@
		""""Module containing some constant heuristic fluid property implemenations.


		Note that the protocol framework is accessed by static type checkes only!

		Warning:

Extending fluid concept to fit CF & Standard formulations #1244

Are you sure you want to change the base?

Extending fluid concept to fit CF & Standard formulations #1244

Conversation

vlipovac commented Oct 21, 2024 • edited Loading

Proposed changes

Types of changes

Checklist

review-notebook-app bot commented Oct 21, 2024

vlipovac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlipovac Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keileg commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlipovac commented Oct 25, 2024

Choose a reason for hiding this comment

keileg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlipovac commented Oct 21, 2024 •

edited

Loading

vlipovac Oct 21, 2024 •

edited

Loading