[tesing#7286] Hnil pr/well primary variable scaling - #7374
Draft
GitPaean wants to merge 5 commits into
Draft
Conversation
The bhp column of the well D block sits 6-7 decades below the others: the
conservation-equation derivatives w.r.t. bhp are ~1e-7 because bhp is in
Pascals, while the control equation contributes d(bhp)/d(bhp) = 1. Measured
cond(D) medians are 9.7e8 (SPE1CASE1) and 1.3e10 (SPE1CASE2_THERMAL), fully
removable by diagonal scaling - i.e. units, not physics.
Scale only the derivative when the well Evaluations are created:
createVariable(totalNumEq, value_[eqIdx]/s, numEq+eqIdx) * s
The stored value_ stays physical, so update(), copyToWellState(), the
absolute bhp lower limit, the convergence checks and getPrimaryVars are all
untouched; only the Newton increments convert back (they arrive in scaled
units). B, C, D and resWell_ stay mutually consistent because they are all
assembled from the same scaled Evaluations, and C D^-1 B is exactly
invariant under the column scaling - confirmed: iteration counts are
unchanged (437 -> 437, 72 -> 73).
--well-bhp-scaling=8388608 (2^23, ~84 bar) equilibrates: cond(D) median
drops to 3.8e3 / 3.7e3 and min |det| moves from 2.9e-11 to 2.4e-4, seven
decades further from matrixblock.hh's absolute 1e-40 branch. The gain is
robustness (float builds, absolute thresholds), not iterations.
--well-rate-scaling exists for symmetry; the rate column measures as
already O(1). Defaults are 1.0 and reproduce unscaled results bit for bit.
A per-well scale derived from the current bhp was tried and rejected:
better median conditioning but ~8x worse in the tail, because stopped and
zero-rate wells are not scaled by their bhp magnitude - and the tail is
what the scaling exists for.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same design as for standard wells: the stored values stay physical and only the Evaluation derivative carries the scale, so B, C, D and the Schur complement remain mutually consistent by construction. The Newton increments for segment pressure and total rate arrive in scaled units and are converted in updateNewton(), where the physical limits (max_pressure_change, the bhp lower limit) then apply unchanged. Fractions and the temperature variable keep scale 1. One consumer needed a matching fix: the classic CPRW coarse well row builds its column entries from C's segment-pressure column, which carries the scale, while the default row-sum diagonal estimates the physical bhp derivative from unscaled B entries. Without the same factor the coarse well column and diagonal disagree by the scale, which doubled linear iterations on BASE2_MSW_HFA (30 -> 59) and SPE1CASE2_MSW_THERMAL (73 -> 139). With it, scaling is iteration-neutral (30/356/73 with and without 2^23), as the C D^-1 B invariance requires. The contracted-diagonal convention needs no fix since it inherits the scale from D itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The scaling puts value_ and the linear system's unknown in different spaces, which is easy to get wrong - the trivial-equation bug fixed in the previous commit is exactly that mistake. Make the boundary explicit instead of implicit: physicalIncrement() replaces the bare varScale() multiplies, so the two places solver-space quantities enter each class are named, and the contract is stated once next to value_ (including the alternative representation and why it was not chosen). Extend the unit test to pin the contract rather than the representation: eval(i).value() == value(i) and a zero off-diagonal derivative for every slot, value()/setValue() as exact inverses (the getPrimaryVars round trip NLDD relies on), and a Newton step with the absolute bhp floor ACTIVE - the existing case uses a zero increment and never reaches that branch, which is where a missing conversion on a physical limit would show up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
assembleTrivialEq, the zero-rate equation for a SHUT valve, wrote a physical rate as the residual but hard-coded its Jacobian entry as 1. The linear system solves for the scaled primary variable, so the entry has to be d(q)/d(q/s) = s; with a hard-coded 1 the Newton step for that segment comes back a factor s too small and is then multiplied by s on update. Pass the rate as an Evaluation and take the derivative from it, so the entry is correct by construction rather than by remembering to multiply. This was the only hard-coded derivative in either well assembler; every other entry already comes from an EvalWell and scales automatically. Latent until now because --well-rate-scaling defaults to 1 and every measurement so far used the default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitPaean
added
the
manual:irrelevant
Member
Author
|
jenkins build this failure_report please |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
testing #7286