OPM
Fix for relative change (used in time step controllers)
This is a new and improved version of #4856. It adds the choice between using just pressure or saturation, or both, when calculating the relative change used in the time step controllers.
The current master adds pressure and saturation together in such a way that the saturation effectively doesn't do anything. Here, I've chosen to use only pressure as default, and I think this will give results more or less identical to what the current master gives.
Question: Might it make more sense to return the square root of the relative change (to get something resembling an L2 norm)?
CI is in broken state.
Okay. Is there a way to unbreak it within a reasonable timeframe?
merge the missing opm-tests PR. i pointed it out but no response yet, https://github.com/OPM/opm-simulators/pull/5937
Concerning this PR: It might make more sense to take the square root before returning the relative change. Also, would it be better to weight the contributions from each cell by their volume (this is easy to implement)? Performing these changes will, however, change the default much more than the current PR does. What do you think, @atgeirr?
What do you think, @atgeirr?
I think you should make the changes you think are correct.
If that changes behaviour too much for existing cases (for example resulting in much shorter average timesteps than the current code does) we may try to address that by also changing the default parameters used for the method, i.e. tuning to get similar timesteps as currently. If that fails, we may need to keep the existing calculations (or something similar) as the default, and require a command line argument to override, but I really hope we do not need to.
I think you should make the changes you think are correct.
Very well! I'll make the necessary changes soon. When taking the square root, the time-step-control-tolerance parameter will most likely have to be adjusted to avoid getting shorter time steps.
I've now added square root and weighting by volume.
Further: When using "pressure+saturation", only pressure is considered for the first time step, due to issues with very large change in saturation after the initial time step. I've not done any such adjustments when only "saturation" is being used.
The following failures can not be fixed with reference update, needs some parameter adjustment.
mpi.compareParallelSim_flow+3_A_MPI_MULTFLT_SCHED_MODEL2 mpi.compareParallelSim_flow_distribute_z+MSW-SIMPLE-5-SHUT-PERFORATIONS mpi.runSimulator/tuning_tsinit_nextstep
The following three failures were due to not being able to finish running, mpi.compareParallelSim_flow+SPE1CASE2 did not finish running. mpi.compareParallelSim_flow+SPE1CASE2_THERMAL did not finish running
mpi.compareECLFiles_flow_blackoil_float+SPE1CASE1
Posting other failures in the following comments, those can be potentially fixed by updating the reference while it needs some attention due to the deviations.
After discussion, I think @erikhide can try to fix failures in the previous comment first. Other regression failures are essentially due to different time stepping, although it triggers bheavoir changes. Still thinking how to address those.
But fixing the previously mentioned failures can be a good start.
The following tests the parallel running did not run through
mpi.compareParallelSim_flow+SPE1CASE2 mpi.compareParallelSim_flow+SPE1CASE2_THERMAL
Is it the case for your local running? @erikhide
Is it the case for your local running?
Yes, it turns out that they crash for time-step-control-tolerance=0.12, but not for 0.11, which I think is a bit strange; I would expect it to run through when being a bit slacker on the tolerances. I've changed to 0.2 now, which works for both models when I run them locally.
Yes, it turns out that they crash for time-step-control-tolerance=0.12, but not for 0.11, which I think is a bit strange;
I did not go into the details of the PR, but I think it sounds a little bit worrying.
With the latest branch, https://ci.opm-project.org/job/opm-simulators-PR-builder/8270/ it looks like the only parallel failure is the following one,
https://ci.opm-project.org/job/opm-simulators-PR-builder/8270/testReport/junit/(root)/mpi/compareParallelSim_flow_3_A_MPI_MULTFLT_SCHED_MODEL2/
Most likely we can relax the comparison tolerance if the well curve comparison looks good.
Value index = 67
(first value, second value) = (9.12757e+08, 9.14413e+08)
Program threw an exception: [/var/lib/jenkins/workspace/opm-simulators-PR-builder/deps/opm-common/test_util/EclRegressionTest.cpp:227] Deviations exceed tolerances.
The absolute deviation is 1655232, and the tolerance limit is 0.02.
The relative deviation is 0.0018101587680645803, and the tolerance limit is 0.001.
I can check other regressions later today or tomorrow. those are regression failures can be potentially fixed through updating the reference if agreed with.
Since the PR introduce new possible options, it is good change some regression tests to use different --relative-change-approach. otherwise some code will not be tested from this PR.
I did not go through all the cases, but for the beginning parts I have been looking at, quite a few cases did not run through with the current version of the PR.
The following is the ones I looked at just now, (based on the order in the Build Artifacts)
01_waghyst1 not finish
02_spe12 not finish
03_spe12_polyhedralgrid
04_spe1case2_thermal_watvisc
05_spe1case2_thermal
08_spe1_float
potentially more, while I did not go through.