pmgbergen / porepy

Python Simulation Tool for Fractured and Deformable Porous Media
GNU General Public License v3.0
251 stars 88 forks source link

RENEWED: Extending fluid concept to fit CF & Standard formulations #1252

Closed vlipovac closed 2 weeks ago

vlipovac commented 3 weeks ago

Closes #1240 Closes #1247

Replaces #1244 for reasons of clarity.

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 in #1244.

porepy.compositional.ChemicalSpecies was merged into FluidConstants, including data class functionality. This led to some changes for SolidConstants as well for inheritance reasons.

The default model set-up is changed to provide a 1-phase, 1-component fluid, where the single component is based on the user-provided FluidConstants in the model.params. The interface to various fluid properties (e.g., fluid_density) is replaced by the general Fluid class.

Types of changes

Checklist

review-notebook-app[bot] commented 3 weeks ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

vlipovac commented 3 weeks ago

I reworked the concept of constant reference pressure and temperature values:

I think a test dedicated to the reference values alone, the evaluation of the perturbation from reference and so on, is called for.

keileg commented 3 weeks ago

@vlipovac: Regarding the tutorials, could you run these on a system with pypardiso installed, so that we don't introduce more error messages in the notebook output?

vlipovac commented 3 weeks ago

@vlipovac: Regarding the tutorials, could you run these on a system with pypardiso installed, so that we don't introduce more error messages in the notebook output?

Done. There is one last error after the last Python cell in stress_discretization,ipynb coming from a matplotlib.Axes object, which I am not sure how to remove.

vlipovac commented 2 weeks ago

The latest changes involve:

Minor open todos:

  1. Decide on whether to leave compositional.materials.Constants and compositional.materials.ReferenceVariableValues there or find a better spot (maybe porepy.utils?)
  2. Add parametrized test for children of Constants, check for freezing of constants and proper unit conversion (though the conversion is to some degree tested separately in each model)
  3. Add test for parsing and evaluation of perturbation_from_reference which now accesses the reference variable values.

If you think one of the last two points is not necessary, please let me know.

keileg commented 2 weeks ago

Thanks @vlipovac. I'll have a look now.

keileg commented 2 weeks ago

@vlipovac The changes look good to me, I have no further comments to that part.

As for the remaining todos:

  1. I prefer not to put things into pp.utils, so for my part, the classes can stay unless someone comes up with a much better suggestion.
  2. Yes, some simple testing would be useful here.
  3. Yes please.

There are three unresolved conversations that all seem to point to the creation of one (or more?) issue. I suggest we make this and then resolve. Please let me know if I misread some of the conversations.

vlipovac commented 2 weeks ago

I think we are done.

  1. Added test for perturbation from reference method in tests/models/test_constitutive_laws/test_perturbation_from_reference
  2. Added parametrized test for all Constants classes available so far in tests/compositional/test_materials (formerly called test_material_constants)
  3. New issues created resulting from review here #1259 #1260

Constants and ReferenceVariableValues are left for now in compositional/materials. But in future we might want to put these two somewhere else (also having #1249 in mind)