Add nonlinear solver based on domain decomposition method#3764
Conversation
|
Note: needs OPM/opm-grid#537 and OPM/opm-models#682 to compile and run correctly. |
| bool matrixEqual(const Matrix& m1, const Matrix& m2) | ||
| { | ||
| // Compare size and nonzeroes. | ||
| if (m1.N() != m1.N()) return false; |
There was a problem hiding this comment.
Wow - that was hard to read 👓
There was a problem hiding this comment.
Not to mention the fact that unless m1.N() is NaN, this condition will never be true. I suspect one of the m1s should be an m2.
There was a problem hiding this comment.
As the commit message says: untested. With a test you would have discovered the m1 != m1 issue which @bska pointed out (I did not see it). This is new code - not deeply entranched in the existing patterns - no reason to not make a test.
There was a problem hiding this comment.
And furthermore: Why not make a separate PR of these matrix utilities right away?
There was a problem hiding this comment.
Ouch! I might have made a test, and a PR, but this part is code thrown together in a debugging process, and not really intended for the end product. Still embarrassing to see...
|
Can this be closed? |
I don't think so. We should rather pick this up after the 2022.10 release, because the feature is well worth having. |
5b34410 to
dea9a40
Compare
2552387 to
a352c93
Compare
ef93c6c to
75feb05
Compare
4c53729 to
8f549d1
Compare
|
Marking this as ready for review. |
|
jenkins build this please |
bska
left a comment
There was a problem hiding this comment.
There are a number of new warnings, mostly concerning unused parameters or unused variables, in this version of the patch. There are also a few regression failures which must be analysed.
While I think the overall patch is desirable, I think we need to do a bit more work before we enable this in the main simulator.
|
jenkins build this please |
|
jenkins build this please |
|
Just a bit of a "heads up" notice: In local testing it seems that this PR breaks As an example, if I run If we want to enable this then we should probably ensure that we don't break useful options in this way. |
|
jenkins build this please |
|
jenkins build this please |
|
jenkins build this please |
Correction: It's the combination of If it does not make sense to have that combination then we need to intercept it earlier so that users don't get the simulator crash. |
Indeed it does not make sense, so yes, we need to check for that. |
Also do not change getting intensive quantities in convergence checks.
|
jenkins build this please |
bska
left a comment
There was a problem hiding this comment.
Thanks. I think this looks good now for the most part. I have just a couple of small comments concerning disabled statements and then I'm prepared to merge this.
Incidentally, while testing this on my local system, I think I may have stumbled upon a problem in GCC 9.x's -O3 optimizer. Comparing the make test output to the master branch at -O3 produces many test failures, but everything passes at -O2. Since GCC 9.x is nearing its end-of-life I'm not too concerned about this however.
| #include <ebos/eclproblem.hh> | ||
| #include <opm/models/utils/start.hh> | ||
|
|
||
| #include <opm/simulators/timestepping/AdaptiveTimeSteppingEbos.hpp> |
There was a problem hiding this comment.
Tiny nit: We also include this header a few lines down. It doesn't really matter since we have include guards, but maybe we could remove one of the include statements?
| #include <opm/simulators/flow/NonlinearSolverEbos.hpp> | ||
| #include <opm/simulators/flow/BlackoilModelParametersEbos.hpp> | ||
| #include <opm/simulators/wells/BlackoilWellModel.hpp> | ||
| #include <opm/simulators/aquifers/BlackoilAquiferModel.hpp> | ||
| #include <opm/simulators/wells/WellConnectionAuxiliaryModule.hpp> | ||
| #include <opm/simulators/flow/partitionCells.hpp> | ||
| #include <opm/simulators/flow/SubDomain.hpp> | ||
| #include <opm/simulators/flow/countGlobalCells.hpp> | ||
| #include <opm/simulators/utils/DeferredLoggingErrorHelpers.hpp> | ||
| #include <opm/simulators/linalg/extractMatrix.hpp> |
There was a problem hiding this comment.
Is there a reason for this ordering? We're mixing simulators/flow, simulators/wells, simulators/aquifers, simulators/utils, and simulators/linalg which at least superficially draws the eye.
There was a problem hiding this comment.
I think this was due to a rebasing error. I have removed all duplicates and avoid adding new headers that are not needed, and inserted all new ones in sorted order.
| std::vector<int> domain_order(domains_.size()); | ||
| if (param_.local_solve_approach_ == "gauss-seidel") { | ||
| // TODO: enable flexibility and choice in choosing domain ordering approach. | ||
| if (true) { |
There was a problem hiding this comment.
Will this ever be anything other than true? If not, we can probably save a level of nesting.
There was a problem hiding this comment.
I would like to keep the extra code with the TODO above as a reminder to look into this, if that is ok.
| for (int ii : {0, 1}) { | ||
| if (std::isnan(res[ii])) { | ||
| report.setReservoirFailed({types[ii], CR::Severity::NotANumber, compIdx}); | ||
| report.setReservoirFailed({types[ii], CR::Severity::NotANumber, compIdx});//, cell[ii], res[ii]}); |
There was a problem hiding this comment.
Will we ever need cell[ii] and/or res[ii]? If not I think we should remove those disabled quantities.
|
jenkins build this please |
On second thought it's probably |
bska
left a comment
There was a problem hiding this comment.
I've reviewed this now and tested it locally. With the caveat that this seems to trigger an instability when compiled with GCC 9.x and -march=native and -O3 I'm sufficiently convinced that it is not the PR itself that causes the problem. I will run a final build check just to be sure and then merge once the build check is green.
|
jenkins build this please |
Build check is green. I'll merge this into the master branch now. |
Making draft PR to provide a place to discuss aspects of the code. It still contains a lot of hacks, and when the time comes to get the functionality into the master branch it will be refactored to reduce the size: many methods have been added to support local solves that largely duplicate existing methods. I also think that there will be a series of smaller PRs instead of one large PR. There should be no change in performance or results for the existing fully implicit Newton(-like) method from the addition of new methods by themselves, but there could be some small improvements here and there. Any such will also be done in separate PRs.