-
Notifications
You must be signed in to change notification settings - Fork 91
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
Use proper mixins in concrete models #1317
Conversation
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.
Looks very good to me. One substantial comment, we can discuss in person if necessary. Also, let's run all tests for good measure. Withholding approval to remind us of that.
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.
Great work. If you're confident about the one comment I made, please feel free to merge.
@IvarStefansson I requested another review, due to some changes which should be inspected closer. Upon closer inspection, the code could be simplified, as well as the inheritance tree of the Terzaghi and Mandel models. The main insight I gained from this (and from looking into the intermediate "solution strategies" for various benchmarks and manu models) is that they are not actual solution strategies in the original sense. They just add some stuff and like exact solutions, initial conditions and some model assertions. Most of it does not deserve the status of a solution strategy, but should maybe in the future be shifted to the actual model class, since it is only valid and used there (in short: exact solution, model assumptions, are currently declared as "solution strategy" mixins, but the code would be cleaner and simpler if those things are with the actual model class). |
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 relatively minor questions.
Closes #1260
Proposed changes
Models contained in porepy, excluding base models in
porepy.models
, use now proper mixins forto eliminate circular paths in the inheritance tree of PorePy models.
Circular paths which I could not break up are the following two:
DataSavingMixin
: Though all base models inherit fromDataSavingMixin
, models intests
andporepy.examples
usually needVerificationDataSaving
which inherits fromDataSaving
. To break those circular paths (if desired), the simplest solution would be to have all base models inherit fromVerificationDataSaving
instead ofDataSaving
. Then the individual verification setups can be turned into proper mixins. Please comment if you wish to do so in this PR.VerificationDataSaving
is resolved, and the abstraction ofInitialConditionMixin
from CF Merger #3: Compositional Flow models #1236 is included.Types of changes
The changes here aim to form the inheritance trees of porepy models into star-like graphs, to a degree possible considering above notes.
Example: Mandel Biot (sorry for the messy graphs, the rendering engine is hard to configure)
Before:
After:
Optimal in theory (with "mixin" denoting a proper mixin):
Breaking change, since model components are now turned into proper mixins and do not work on their own (which was the case in some tests for custom geometries)
Checklist
Put an
x
in the boxes that apply or explain briefly why the box is not relevant.pytest
was run with the--run-skipped
flag.