Also solve std for every inner iterations as default #6098
Conversation
|
jenkins build this failure_report please |
|
vow, then my guess is that it improves the well solve for the well under THP target? |
| @@ -851,7 +851,7 @@ namespace Opm | |||
|
|
|||
| // only use inner well iterations for the first newton iterations. | |||
| const int iteration_idx = simulator.model().newtonMethod().numIterations(); | |||
| if (iteration_idx < this->param_.max_niter_inner_well_iter_ || this->well_ecl_.isMultiSegment()) { | |||
There was a problem hiding this comment.
If we go this direction, we probably want to do it differently here.
iteration_idx by default will be maximum 20. We probably can remove this if condition or using some other if condition if I did not read the code wrong.
There was a problem hiding this comment.
I think it could be useful to keep the option of not solving with inner iterations for all Newton iterations.
|
I have started testing the Norne prediction cases with STW, and the results are not conclusive. In some cases, Newton's convergence gets worse. I will have to dig deeper into these cases to understand what happens. |
Norne had this problem from many years ago, not sure whether it is still the case though. |
Note that some of the Norne prediction cases fail when using the current master. I've tested alternative time stepping approaches which seem to work better (i.e. the simulation doesn't fail). An early version is available using |
|
benchmark this please |
|
jenkins build this failure_report please |
|
I think we should try to get this in before the release, as not solving the well with inner equations may lead to the stagnation of simulations. |
|
Some spikes in the results are the most worrying differences. I have tried to reproduce them locally: From my point of view, this should be merged. We would like to use the new local solve approach in the simulator at every iteration, as well as for standard wells. |
|
@GitPaean, what do you think? Should we merge this before the release? I think so, as it improves the convergence in most cases. If it doesn't, it points to other issues that must be solved. If this leads to a problem for particular models, we have a simple workaround of passing the old default parameters. |
|
jenkins build this failure_report please |
|
let me have a look. It was mentioned that it made some running worse, was there any update regarding that? |
|
I confirm that But it is a weird case, that INJ1 is either GAS or WATER injection history matching mode, while the most of the time (or all the time of reference results), WGIP for INJ1 is zero, which is rather strange. DBG and PRT file does not mention anything related to this. |
|
Did you check the results related to UDQ_WCONPROD? I confirmed the jenkins comparison locally. |
|
And also did not check all of the regression tests. |
It seems super-sensitive to small perturbations in the time-stepping. I will dig deeper to see if I can solve the underlying issue. |
|
Using --check-group-constraints-inner-well-iterations=false (or applying #5924) seems to fix 02_udq_wconprod (only time-stepping differences remain, visually identical results with shorter time steps). |
|
I think the issue is related to control switching. #6145 seems to fix it. However, so does restricting the timestep and reducing the maximum number of well switching per step. So I could just be random, but I don't think it is directly related to this PR. i.e., I think we can merge this and update the data. I also tagged #6145 for the release. It fixes an output bug, but we may need to change the default maximum value of switching as we expect more switching during local solves. |
|
@vkip comment strengthens my suspicion that it is related to well-switching. |
|
Let's merge #5924 first. |
Sounds good, hopefully many of the test failures here disappear after that. |
|
There are still some issues for Norne that need to be solved before this can be merged. I will keep it open, but remove the release tag. |
In master, we always solve inner iterations for MSwells but not StdWells. The lack of inner iterations for StdWells can lead to convergence issues for Newton. This changes the default value to 99 and removes the check for MSWells, thus allowing not to solve MSWells during some Newton iterations.