Skip to content
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

CF merger #2: introducing porepy.compositional #1186

Merged
merged 629 commits into from
Jun 26, 2024

Conversation

vlipovac
Copy link
Contributor

@vlipovac vlipovac commented Jun 8, 2024

Proposed changes

Introducing modeling of multicomponent and multiphase mixtures.
Introducing also mixin classes which enables user to easily create mixtures and introduce a attribute fluid_mixture into the model.

Things to discuss:

  1. I prepared a test for singular mixtures, testing that unnecessary variables are not created in the case of 1 component or 1 phase or both. But this is non-trivial, if we allow phases to have different components (so not every component has to be in every phase). I did not make any restrictions here, since this "unified assumption" is only necessary in the unified flash. I need your thoughts on this.
  2. Integration with models and the existing fluid constants: Two options:
    a. make functionality to build a single-phase, single-component mixture with some simplified EoS from fluid constants.
    b. replace fluid with the fluid mixture and start transitioning the model framework to a more general setting. On a side node, a similar discussion will be required when introducing the compositional flow equations (relation to current single phase flow etc).

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:

Unrelated changes: Overload of logical operators in forward_mode.AdArray. I think this is required in some point of the new code.
Bugfix: Fixing memory leak in surrogate operator (require depth parameter for shifting values in time and iterate sense, otherwise excessive storage)

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).
  • If new skipped tests have been introduced in this PR, pytest was run with the --run-skipped flag.

For the record

This PR contains some fixes in numba-compiled functions (related to casting and signature typing), which were discovered in some completely unrelated context.

vlipovac added 30 commits June 1, 2023 16:57
MOD: Structuring of initialization
MOD: Added h-v- flash to scripts for first publication
MOD: Re-namedsome functionality in chemical interface
FIX: Implementation of BIPs for components with custom models
@vlipovac vlipovac requested review from keileg and OmarDuran June 21, 2024 07:01
Copy link
Contributor

@keileg keileg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments only, I believe. One mentions tests, but that we have discussed already.

src/porepy/compositional/base.py Outdated Show resolved Hide resolved
src/porepy/compositional/base.py Outdated Show resolved Hide resolved
tests/numerics/ad/test_forward_mode.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
src/porepy/compositional/compositional_mixins.py Outdated Show resolved Hide resolved
src/porepy/compositional/compositional_mixins.py Outdated Show resolved Hide resolved
src/porepy/compositional/compositional_mixins.py Outdated Show resolved Hide resolved
@vlipovac
Copy link
Contributor Author

vlipovac commented Jun 24, 2024

@keileg
I have added tests for the utility functions in tests/compositional/test_utils.py

Could you please go through all unresolved conversations and mark them resolved if you are satisfied with the changes?
All of them should be covered by the changes, except the discussion on the changes in requirements.txt
We discussed this in person mostly.
Regarding the dependency on chemicals, I have no strong opinion. If you want it to be a weak dependency, I only have to change some tests where I (for convenience) created components using the API function.

@vlipovac
Copy link
Contributor Author

@keileg
chemicals is now a weak requirement.
If the user installs it, the method load_species is usable.
The subpackage works without it, tests were adapted to not rely on load_species.

Copy link
Contributor

@keileg keileg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

@vlipovac vlipovac merged commit b4d890f into develop Jun 26, 2024
7 checks passed
@vlipovac vlipovac deleted the cf_merger_2_fluid_mixture_modelling branch June 26, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants