-
Notifications
You must be signed in to change notification settings - Fork 131
Two-point stress approximation (TPSA) geomechanics #6650
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
base: master
Are you sure you want to change the base?
Conversation
|
I'll just make one general comment here and then I'll be quiet. Adding 6,600 lines of new code in a single PR, especially since there's hardly any unit tests, is usually not the best way to go about making the PR easy to review. Is there no way to split this up into smaller parts, ideally with documentation and unit test coverage for each part? |
|
Since it's entirely a one way dependency, how about we extract the block size 7 changes and do those first? |
I have made a new PR #6660 with only the block size increase. Thanks for the suggestion @akva2! The number of GPU-related files that needed to be modified was more than I thought initially! I will rebase this PR once the block size PR is merged.
Thanks for the comment @bska. I have added some additional unit tests, including the PR in opm-tests OPM/opm-tests#1440. I hope this covers and shows the workings of the TPSA code better. I can also add that TPSA, structurally speaking, is (almost) a straightforward copy of TPFA in Flow. I hope this will make it easier to review. |
Thanks for adding tests, that certainly aids in the review. Tangentially, my own personal goal when I'm adding new code is to cover the public interface of each new class and public function with a comprehensive set of unit tests. I also try to make adding each class/public function into a separate PR and then having an integration PR at the end that ties everything together and activates the new logic. I don't always achieve this goal but it feels good when I do. |
|
An issue I have found is that the If anyone has any ideas how to get the TPSA parameters in |
|
Sadly this is a limitation of the system. The same applies to other model specific options, such as solvent. To make it work, you have to register the parameters manually, since, as you allude to, they are not registered through the SimulatorFullyImplicitBlackoil::registerParameters() |
|
I had a brief look. This seems very good new contributions. It is an alternative to part of opm-flowgeomechanics, which do vem discretization + some fracturing. You have done a very good job at getting things into the system with models +++, which is partly avoided in opm-flowgeomechanics. Also maybe the work in opm-flowgeomechanics should be changed a bit to be more like "opm" interface as the tpsa uses. Some comments:
|
Thanks for the hint! I have included linear solver and newton parameters for TPSA directly in |
Thanks for the feedback, @hnil. I appreciate you taking the time to have a look at this.
Yes, there may be some classes which I have name somethingTPSA (like
We are still figuring out some of the outputs (particularly stress and strain) and boundary conditions (particularly Robin conditions). It would be worth syncing up with opm-flowgeomechanics, in general, to avoid duplicating code!
Thanks, I will look into hypre-based solvers! ISTL solvers have worked great, at least on the examples we have run so far, which I think is due to the nature of TPSA being closely related to TPFA. |
totto82
left a comment
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.
Thanks for the work. Most of my comments are minor. My main concern is related to code duplication (also raised by @hnil) . There are many files that are essentially copies of other files for TPFA. Can some of these copies be avoided? Let's discuss it file by file (in a meeting?)
|
|
||
| /*! | ||
| * \brief Calculation of (linear) elasticity model terms for the residual | ||
| */ |
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.
Add the equations that are solved. (+ ref to paper?)
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.
done
| it under the terms of the GNU General Public License as published by | ||
| the Free Software Foundation, either version 2 of the License, or | ||
| (at your option) any later version. | ||
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 essentially a copy of tpfalinearizer.hpp. Can the TPFA code be re-used?
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.
Unfortunately there are a few fundamental differences with the linearization, so I could not make it work with tpfalinearizer.
|
|
||
| /*! | ||
| * \brief Primary variables in (linear) elasticity equations | ||
| */ |
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.
Add what the primary variables are
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.
done
| { | ||
| SymTensor val; | ||
| return val; | ||
| } |
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.
Why is this not implemented? (and below)
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.
As far as I understand, many of the variables are outputted as cell center variables, and computing these with TPSA is a bit of challenge as of now. But we should definitely add, at least stress and strain, when we figure out how to compute them.
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.
It maybe a better way, but a least square approximation based on forces on faces should at least be one way.
| */ | ||
| template <class TypeTag> | ||
| class TpsaNewtonConvergenceWriter | ||
| { |
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.
Why is this needed?
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.
It was unnecessary, so it is now removed
| */ | ||
| template <class TypeTag> | ||
| class TpsaNewtonMethod | ||
| { |
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 just Newton. Why can't you use newtonmethod.hpp directly (or at least inherit from it?
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.
Unfortunately, and as far as my understanding, I could not inherit the standard NewtonMethod class (from opm/models/nonlinear) because I needed to override the "using" statements with TPSA related properties. At least, when I tried, the Newton method called Flow specific linearizer, linear solver, etc, instead of the TPSA specific functions and classes.
I have, however, done a refactoring of the TPSA Newton code to remove all unecessary and unused 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.
In principle you could make a separate typetag just for the newton related types. You can splice in a 'default model' of this to the default typetag, so nothing changes in "regular" code, but replace it in the instance of the newton method for the tpsa model. Whether it is worth it, I make no judgement on, just a how it could be done.
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.
Thanks for the tip! I will at least give it a try. Are there any examples of splicing typetags in the code already that I can look at for reference?
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.
You can for instance have a look at fvbaseproperties.hh and how the linearsolver settings is spliced in.
The gist of it is that you have a splice, you assign types to the splice, then this is spliced into the main typetag so you can request those types from the main typetag.
| outside.faceCenter = inside.faceCenter = geometry.center(); | ||
|
|
||
| // OBS: Have not checked if this points from cell with lower to higher index! | ||
| faceNormal = intersection.centerUnitOuterNormal(); |
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.
Is this comment still valid?
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, I have not tested on grids other than CpGrid. I replaced the function content with a throw, since I do not know if TPSA works properly with for example polyhedral grid.
| * \param simulator Simulator object | ||
| */ | ||
| ISTLSolverTPSA(const Simulator& simulator) | ||
| : simulator_(simulator) |
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.
Can you inherit / use the standard ISTLSolver here?
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 could not inherit ISTLSolver class (for the same reasons as with Newton), but I changed the code to inherit AbstractISTLSolver. That required a small change in templates for AbstractISTLSolver, which I hope does not break anything!
I have also removed the setupPropertyTreeTPSA function, and just use setupPropertyTree instead with a small change in input.
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.
For a way to separate the code almost as much as possible see https://github.com/hnil/opm-flowgeomechanics
- TPSA-Flow coupling done at the nonlinear solver step level: - -- Lagged coupling -> TPSA lags behind one time step - -- Fixed-stress -> Iterate until Flow and TPSA converge - Generic TPSA solver using Newton with associated linearizer. - Linear elasticity equations implemented - Specialized version of ISTL linear solver for TPSA - Output to restart files and VTK
Addition of a cross product term for rotation equations.
Template change in AbstractISTLSolver necessary to generalize class.
Linear solver setup used setupPropertyTree, so new function unecesary. New boolean to avoid Flow-specific linear solver presets.
- Removed unnecessary (null) convergence writer - Removed constraints - Removed unused/empty functions - Removed auxiliary module calculations - Added verbosity level with runtime parameter
This PR introduces the implementation of TPSA geomechanics solver with coupling to Flow. Most important features are:
There are blackoil, gas-water dissolution (mostly for CO2-/H2STORE) and one-phase simulators implemented, but in principle any other simulator can be generated as well.
Reference to implementation paper can be found here.
Putting this PR in draft mode since it is rather large, but any feedback/comments/discussion on the implementation is welcome at this point!
Companion PR: OPM/opm-common#4846