OPM
Revise Mixture Density Method for No-Flow Producers
This PR switches the approach introduced in commit eeb1b7e36c9ae3c30ba5d262fa623dc0eea2e10b (PR #3169) to using a mobility weighted average of cell level densities for the connection level mixture densities in no-flow producing wells. We also use the recent stoppedOrZeroRateTarget() predicate to identify those no-flow producing wells instead of inspecting the connection flow rates.
The mobility weighted average gives a more monotone pressure buildup for the stopped wells and this is usually what the engineer wants. This revised approach furthermore needs fewer cell-level dynamic properties so simplify the computeProperties() signature by introducing a structure for the property callback functions and update the callers accordingly.
While here, also refactor parts of the StandardWellConnections code to aid future maintenance, mostly by splitting out parts of the existing computePropertiesForPressures() member function to several other helper functions.
I've gone through the regression test failures and most appear to be triggered by (slightly) different time steps. It's not clear to me, however, whether the solutions generated by the current master sources are "better" than those generated by this PR. For what it's worth, this PR does seem to give more stable WBHP behaviour in the shut-in period prior to the 1st of April 1999 in NORNE_ATW2013_B1H_REPERF than the current master sources. PR #4817 also discussed this stability issue.
The PR has been tested on the test model and also on a field model. The results are excellent and I'm looking forward to see this going into master.
The PR has been tested on the test model and also on a field model. The results are excellent and I'm looking forward to see this going into master.
Thanks a lot for testing the PR–that is greatly appreciated. We have a few work items that must be completed before we're ready to merge this, but we will prioritise getting this work into the master branch once those have been completed.
It is great this PR fixes certain reported cases.
My main comment is that, it is a little unfortunate that different formulation is used for exact zero-rate control wells.
Is it possible that the same formula can actually be used for other normally running wells? If it is true, then it might means it does not change the results significantly enough so that is why we have been using different formula for normally running wells.
As part of investigation, how about the wells with non-zero while very small rate? for example, 0.1 m^3 / day for oil production. It will use the original formula in the master branch, but theoretically, it might should be similar to the zero rate controlled wells when coming to be actual values?
My main comment is that, it is a little unfortunate that different formulation is used for exact zero-rate control wells.
I don't disagree, but even the current master sources use a separate procedure for no-flow producer wells. That procedure was introduced in #3169. Other than the different averaging procedures, the current master sources uses a much more sensitive predicate for identifying no-flow producing wells where even a single reservoir connection having a flow rate of 1.0e-20 SM3/day is enough to classify the well as flowing instead of stopped. The stoppedOrZeroRate() predicate is much more practical in this respect.
Is it possible that the same formula can actually be used for other normally running wells?
That's a good question, and the answer is that I don't know, although I suspect it is not. It might be interesting to investigate that as follow-up work.
As part of investigation, how about the wells with non-zero while very small rate? for example, 0.1 m^3 / day for oil production. It will use the original formula in the master branch, but theoretically, it might should be similar to the zero rate controlled wells when coming to be actual values?
That's a good question as well.
jenkins
Yeah... The current PR breaks a number regression tests. I've been investigating the failures to understand whether or not this introduces any unforeseen consequences and there does seem to be some impact on the timestep selection algorithm. If I impose a maximum timestep of one day, e.g., through --solver-max-time-step-in-days=1 I can make this PR match the solutions generated by the current master branch. On the other hand, with unrestricted timestep selection we do get differences.
I think I'd like to generate new reference solutions and merge this, but I'm open to other points of view.
If I impose a maximum timestep of one day, e.g., through
--solver-max-time-step-in-days=1I can make this PR match the solutions generated by the current master branch
I think this treatment should be done on a case-by-case basis and not universally, as this would make test times grow significantly.
I think I'd like to generate new reference solutions and merge this, but I'm open to other points of view.
I tentatively agree with this. The fact that with one-day timesteps you get a match with the master branch indicates that the changes here do no harm. Have you also verified that the behaviour of the failing tests is reasonable?
If I impose a maximum timestep of one day, e.g., through --solver-max-time-step-in-days=1 I can make this PR match the solutions generated by the current master branch.
I think it is generally fine in my opinion. We had to do this for numerous occasions. Main concern is whether this will increase the running time of the cases to be much longer. If the regression only happens to the first time steps, maybe can enforce the 1-day (or other time step sizes that help) for the relevant time steps either by modifying DATA file or using tuning. With this way, we might gradually stablize many of the regression test cases.
I am not opposed to updating the reference and get the PR merged either if the failures are clearly due to time stepping changes and no behavoir changes occurred.
If I impose a maximum timestep of one day, e.g., through
--solver-max-time-step-in-days=1I can make this PR match the solutions generated by the current master branchI think this treatment should be done on a case-by-case basis and not universally, as this would make test times grow significantly.
Sorry for not being clear. I did not mean to suggest that we alter the test parameters. Rather, I intended (just) to state the fact that if I restrict to the maximum timestep size to one day in both the master sources and this PR, then I can get both versions of the simulator to generate the same–in eyeball norm–numerical solutions.
I think I'd like to generate new reference solutions and merge this, but I'm open to other points of view.
[...] Have you also verified that the behaviour of the failing tests is reasonable?
Yes. Both the master sources and the current PR capture the same "trends" with unrestricted timestep selection, albeit with a small caveat for the cases that involve dynamically triggered action blocks.
I am not opposed to updating the reference and get the PR merged either if the failures are clearly due to time stepping changes and no behavior changes occurred.
Cool. I do indeed think that the differences are related to timestepping differences. That said, I would of course prefer that the timestep selection were unaffected by this work...
The PR's approved, the build check is green and the new reference solutions have been installed on the CI system. I'm merging this now to activate the new procedure.