Take a gradient at a collar psi you supply - #90
Conversation
The shape of it
One composite serves both paths. The stationarity test: refined from what #88 proposedThe issue said the prescribed path should "skip the stationarity test", and the But the number keeps its meaning and gains a better one: how far the collar
At a stationary point that is the analytic zero (#89's envelope). Away from one
Verification
|
aornugent
left a comment
There was a problem hiding this comment.
The design is right and the load-bearing test is the right one. Keeping a single
composite and asserting that psi = psi* with dpsi_dtheta = -M/H reproduces the
solving path with identical() is what makes this a second entry point rather than a
second implementation, and refusing a gradient at a clamped psi — instead of handing
back the direct term, which would be plausible and wrong — is the correct call for the
same reason the pinned case gets the fallback. Returning M, H and dY_dpsi so a
caller can integrate ds/dt = k(M + H s) themselves is a better boundary than trying
to guess the caller's dynamics.
I read the R and C++ at() against each other line by line and they agree
structurally: resid is in scope where the prescribed branch uses it, dY_dpsi is
filled on every path that later reads it, the zeros materialisation in at() is in
the right place, and the dpsi buffer in batch() outlives the Prescribed pointing
into it. Four things below, the first of which is a defect.
1. An integer psi is silently reported as clamped
leaf_gradient() calls
.gradient_check_psi(psi, dpsi_dtheta, method)and discards the return; the helper ends in invisible(NULL) and never coerces. The
clamp test is then
clamped <- !identical(l$opt_root_psi_, psi)and identical(3, 3L) is FALSE. So leaf_gradient(..., psi = 3L) — or the entirely
ordinary for (p in 2:5) leaf_gradient(..., psi = p), since 2:5 yields integers —
returns status = "clamped", an all-NA gradient, and a psi in the result equal to
what was asked for, so the "where it was pulled to" diagnostic shows no movement and
there is nothing to notice.
Three things make this worth fixing rather than documenting:
- the batch counterpart does not have it —
.gradient_check_psi_batch()returns
as.numeric(psi), so the two entry points disagree on the same input; - C++ does not have it either —
util::identical(double, double)isa == b, so the
R reference and the implementation it is required to match bit-for-bit differ in
which inputs they call clamped; - the sibling this cites as precedent uses the type-tolerant form:
.gradient_outputs_at()testsl$opt_root_psi_ != psi.
One line either way: assign the checker's return and have it as.numeric(), or use
!=. Worth a test with a non-double psi — nothing in the new tests passes anything
but a double literal or a value read back out of a previous result, which is why this
survives the suite.
(Found by reading plus checking identical(3, 3L) at the R prompt, not from a built
run — so worth confirming against a build before fixing.)
2. The prescribed path has no no-gradient, and adopts the shut-down sentinel
On the solving path usable == FALSE is a hard stop: dprofit_droot_collar_psi
returns a sentinel zero at a shut-down point rather than a derivative, and the code
says so. On the prescribed path use_ift = !clamped regardless of usable, and then
} else if (prescribed) {
dY_dpsi[["profit"]] <- resid
}adopts that sentinel as if it were dprofit/dpsi. With the default dpsi_dtheta = 0
it costs nothing. With a non-zero one — which is the case this feature exists for — the
profit column silently loses its indirect term at exactly the operating points where a
tracking model spends its dry-patch time. Either the prescribed path needs its own
no-gradient, or there needs to be a test at a shut-down driver with a non-clamped
psi showing the state is unreachable. Right now the Prescribed/Clamped pair is
the whole vocabulary, and usable is computed and then not consulted.
Relatedly, .gradient_dpsi_dtheta checks is.numeric(dpsi_dtheta) && !anyNA(...) but
not is.finite. Inf therefore reaches the composite, where #89's profit line is
direct + rounded(0 * d_psi) — giving ±Inf in four columns and NaN in profit alone.
Cheapest fix is on the #89 side (assign profit's column rather than multiply); a
finiteness check here would also do it.
3. Status reordering, and the default: arm
Error moves from 3 to 5. Source-compatible, not value-compatible, so any consumer
holding the integer breaks quietly. Separately, status_name's default: return "error" means the next member added anywhere before Error gets labelled "error"
rather than failing to compile — an explicit case Status::Error: with no default:
makes the switch exhaustive and turns that into a compiler diagnostic.
4. Minor: method is per call, and its comment says per row
In src/gradient.cpp,
method_used[i] = psi_p != nullptr ? "prescribed" : (r.used_ift ? "ift" : "fd");psi_p is call-level, so the ternary does not vary across i — but the comment above
it justifies the placement as "per row rather than per call because a batch can carry a
psi for every row and still have one throw". A row that threw reports
status = "error" with method = "prescribed". Defensible, but the comment is
describing behaviour the expression does not have.
On the stack
This targets #89's branch, so it inherits that PR's rebase problem: 26574f4 moved
every cell of gradient_golden.tsv and landed a competing expect_golden() tolerance
design. Worth settling underneath before this one moves.
0ea6759 to
e1d8249
Compare
Both implementations called find_root_collar_psi() and read opt_root_psi_ back, so a model that TRACKS the optimum rather than finding it — TF24f carries the collar as an ODE state — got a confident answer about the re-solved optimum instead, with nothing saying so. `psi` evaluates there; `dpsi_dtheta` (default zero) is the indirect term, since an imposed collar's response is the caller's to know. `M`, `H` and `dY_dpsi` come back as the coefficients of their sensitivity ODE. `stationarity` no longer routes but still decides profit's column, so `psi = psi*` with `dpsi_dtheta = -M/H` reproduces the solving path bit-for-bit. A clamped psi reports "clamped" and no gradient: the collar moves with the bound, not with dpsi_dtheta. An infeasible one reports "no-gradient" rather than adopting dprofit's shut-down sentinel, which needed the feasibility flag bound to R for the first time. Closes #88. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
936fecb to
1461b48
Compare
|
All four addressed, pushed as 1. Integer
|
Stacked on #89 — review that one first; this branch targets it.
Both implementations called
find_root_collar_psi()and readopt_root_psi_back, so a model that TRACKS the optimum rather than finding it — TF24f carries
the collar as an ODE state — got a confident answer about the re-solved optimum
instead, with nothing saying so.
psievaluates there;dpsi_dtheta(defaultzero) is the indirect term, since an imposed collar's response is the caller's to
know.
M,HanddY_dpsicome back as the coefficients of their sensitivityODE.
stationarityno longer routes but still decides profit's column, sopsi = psi*withdpsi_dtheta = -M/Hreproduces the solving path bit-for-bit.A clamped psi reports
"clamped"and no gradient: the collar moves with thebound, not with
dpsi_dtheta. Closes #88.