FlowProblem: drop a stray cell-index assignment in updateMaxWaterSaturation_ - #7296
Conversation
hnil
added
the
manual:bugfix
|
jenkins build this please |
|
jenkins build this failure_report please |
|
Correction to my own PR text: I first wrote that SPE1CASE2_ROCK2DTR was byte-identical with and without this change. That was a stale build — it is not. Rerun cleanly: master 866 Newton iterations, this branch 583. UNSMRY and UNRST differ, INIT does not. So the bug is not latent at all; it makes that deck materially harder to solve. The one CI failure is that same deck, because the stored reference encodes the buggy result. It needs regenerating. |
…ration_
maxWaterSaturation_ is a std::vector<Scalar> sized by the number of grid
cells and indexed by element index everywhere it is used. It has no
time dimension, so
maxWaterSaturation_[/*timeIdx=*/1] = maxWaterSaturation_[/*timeIdx=*/0];
does not shift a time level -- it overwrites cell 1's running maximum
with cell 0's. Since the per-cell update that follows only ever raises
the value, cell 1 keeps the wrong maximum whenever cell 0 is the larger
of the two.
The structurally identical updateMinPressure_(), which serves the same
ROCKCOMP path, has no such line.
Only affects ROCKCOMP runs, and only cell 1. SPE1CASE2_ROCK2DTR is
byte-identical with and without this change, because cells 0 and 1 reach
the same maximum there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5be6a08 to
4afc278
Compare
atgeirr
left a comment
atgeirr
left a comment
There was a problem hiding this comment.
This should be merged.
I see something else suboptimal here: Instead of return true on line 1366 (and others) we should use the (false or true) values returned from updateMaxWaterSaturation_() calls inside updateProperty_() and pass the or-ed value all the way out. As is not, we return true irregardless of updating any max saturation.
|
jenkins build this update_data please |
|
jenkins build this update_data please |
Reason: PR OPM/opm-simulators#7296 opm-common = cf5f36dce04b5a7d8bfce4bd5b25e0259e958e03 opm-grid = e94a7386ca9d62104c329214d28e65f2e2da44dd opm-simulators = 5ea8262a692c13c8f831fc2595831cac0a19d36c ### Changed Tests ### * spe1case2_rock2dtr
|
jenkins build this opm-tests=1560 please |
Automatic Reference Data Update for PR OPM/opm-simulators#7296
bska
left a comment
bska
left a comment
There was a problem hiding this comment.
Very good. The new reference solutions have been installed on the CI system so I'll merge this to bring in the fix.
4 participants
maxWaterSaturation_is astd::vector<Scalar>sized by grid cells and indexed by element index everywhere it is used (FlowProblem.hpp:1170,:1395,:1594,:1845,FlowGenericProblem_impl.hpp:621). It has no time dimension, sodoes not shift a time level — it overwrites cell 1's running maximum with cell 0's. The per-cell update that follows only ever raises values, so cell 1 keeps the wrong maximum whenever cell 0 is the larger of the two.
updateMinPressure_()is the structural twin on the same ROCKCOMP path and has no such line.Effect
Larger than expected. On
spe1/SPE1CASE2_ROCK2DTR:Corrupting one cell's water-saturation maximum feeds a wrong rock-compaction multiplier into that cell for the rest of the run, and the solver pays for it.
compareECLFiles_flow+SPE1CASE2_ROCK2DTRfails on this branch — it is the only failure out of 580 — because the stored reference was generated with the bug present. The reference needs regenerating in opm-tests; happy to open that PR if this is agreed.Context
Found while auditing #7198. This line is the sole reason that rolling
maxWaterSaturation_back across a failed timestep changes results: every other quantity in that snapshot is a plain monotone extremum recomputed from the restored solution, hence idempotent, and each measures inert when its restore is removed. This one is not, purely because of this assignment.