Record how plant #576 resolved: check the caller before clamping - #4
Open
dfalster wants to merge 1 commit into
Open
Record how plant #576 resolved: check the caller before clamping#4dfalster wants to merge 1 commit into
dfalster wants to merge 1 commit into
Conversation
The spline worked example asked "clamp or widen?" and left it open. The answer was neither, and the reason generalises, so the entry now records it: the lookup was refusing a stem potential wetter than saturation, asked for from the hydraulic shut-down state that the feasibility analysis one level up had already rejected. Clamping would have returned a plausible number for an impossible state -- the failure mode the table's Never row already warns about -- and widening was aimed at the wrong end of the domain. Two review questions added from it: is the guard in the right place (a guard where a failure surfaces can be correct code in the wrong file), and where a state is handled in two places, do they agree -- the bug was an asymmetry between two gradient methods, one of which took the zero-gradient branch in shutdown and the other did not. Also two additions to the reporting half, which paid for itself: name the bracket and not just the point, and do not tighten a NaN comparison on the way past -- `u < lo || u > hi` is false for NaN by design in some callers, so negating it into an in-range test reads as a diagnostic improvement and is really a behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Follow-up to plant #576 / plant#582. The spline worked example in
model-robustness.mdwas written while #576 was open and left the question hanging — "if conductivity really is ~0 out there, clamping is a physical statement… if not, it should fail". It's now answered, and the answer is neither, which is worth recording because the reasoning generalises.What the entry now says
The failing lookup was the inverse transport curve, asked for the stem potential that would carry a negative sap flux from a collar at the critical potential — a potential wetter than saturation, which does not exist. The spline was right to refuse. The caller was wrong to ask, because it was asking in the hydraulic shut-down state that the feasibility analysis one level up had already rejected.
Clamping would have returned a plausible stem potential for an impossible state — precisely what the table's
Neverrow already warns about. And widening was aimed at the wrong end of the domain: the failure was at the lower bound (ψ = 0, saturation), not past the vulnerability curve.Two review questions added
Two additions to the reporting half
That half of the original entry stands and paid for itself immediately — naming the spline, point, distance outside, domain and calling function turned a four-call-site bisect into one read. From doing it:
u < lo || u > hiis false for NaN by design in some callers; negating it into an in-range test reads like a diagnostic improvement and is really a behaviour change. I made that mistake first — it broke a documentedf(NA) -> NAcontract — and reverted it.Docs only; no rule changes, and the decision table is untouched.