NTN: fix the lambda-floor cancellation and the sub-NHN fit (#55) - #57
Open
davidhbernstein wants to merge 1 commit into
Open
davidhbernstein wants to merge 1 commit into
davidhbernstein wants to merge 1 commit into
Conversation
Two defects, both from issue #55, on one branch: shipping the first alone would ship a measured regression. 1. The log-likelihood cancelled at the lambda floor. l4 + l5 was log Phi(aa) - log Phi(bb) where aa = (mu/lam - eps*lam)/sig and bb = (mu/sig)*sqrt(1 + lam^-2) share mu/lam, so with mu < 0 each term reached about -1e13 while their difference stayed O(1) -- the reported value was the rounding error of two enormous numbers. aa^2 - bb^2 is analytic with mu^2/lam^2 cancelling, and against l3 the remaining mu^2 cancels too, leaving -eps^2 (1 + lam^2)/(2 sig^2) + tilt(aa) - tilt(bb). Taken through .log_phi_tilt() when both arguments are negative; the mu > 0 side has no cancellation and keeps the direct path, since the tilt would manufacture one there. Verified against the analytic lam -> 0 limit, which the fix approaches at the expected O(lam^2) rate to 2.2e-16 while the old form turned around below lam = 1e-4 and reached 0.63, and against level-space log(Phi(aa)/Phi(bb)) on 116 grid points where neither Phi underflows. 2. The fit could end far below the nested NHN. At mu = 0 the NTN likelihood IS NHN's, and mu = 0 is interior, so this is a soundness requirement and not a boundary supremum. Fixed on the #45/#30 pattern: a fit with mu held at 0, checked at the end and polished from, not added as a start. On 2700 fits against main 1edf823: reporting above the true value 40 -> 0, on the lambda floor and above OLS (impossible) 9 -> 0, below the nested NHN 200 -> 13 with the worst gap 764 -> 0.39. Scoring both builds' optima under one correct likelihood, the new fit is higher on 278 and lower on 27, for +12123 in total log-likelihood. The 13 that remain are all on the flat sigma_v boundary of A59. mu -> -Inf is a supremum, not a maximum: a continuation profile converges to a finite limit near -368.3862 with lambda and sigma growing in proportion to |mu|. An extra fix that widened the polish window to chase it was measured and reverted, since it only ever stops where the pass cap falls. test-vcov-bhhh-advice.R (#54) needed adapting: its degenerate fixture sat on a flat ridge, and sliding 9e-5 along it left the sandwich bread invertible, so its trigger stopped triggering while the invariant held. It now searches several samples for the trigger and asserts it was exercised. 23 assertions in the new test file fail on main. Full suite: 0 failures, 5368 passing, 43 warnings, 8 skips, 35 min. 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.
Fixes #55. Both defects in that issue, on one branch, because shipping the first
alone would ship a measured regression — see "Why both halves" below.
1. The log-likelihood cancelled at the
lambdafloorl4 + l5waslog Phi(aa) - log Phi(bb)withBoth arguments carry the same
mu/lam. Withmu < 0andlamat its flooreach term reaches about
-1e13while the difference isO(1), so the reportedvalue was the rounding error of two enormous numbers of opposite sign. Nothing
is wrong with either
pnorm()call.The signature is unmistakable. On seed 1011401 /
y_pcs_st, where the fitreached
mu = -32578,sig = 213, the per-observation error came out as anexact multiple of 512 — the floating-point spacing of
aa^2/2at2.3e18—and the total over 200 observations was exactly
200 * 512 = 102400.aa^2 - bb^2 = (-2*mu*eps + eps^2*lam^2 - mu^2)/sig^2analytically, withmu^2/lam^2cancelling exactly; againstl3the remainingmu^2cancels too,leaving
Since
sig_v^2 = sig^2/(1 + lam^2)that leading term is-eps^2/(2 sig_v^2)—structurally the residual #28 left behind in TSL, which is why the two fixes
look alike. Taken through
.log_phi_tilt(), as"NE","NGE"and"TSL"already do.
Only when both arguments are negative. With
mu > 0both go to+Inf,log Phiof each goes to0, and the original expression has no cancellation —there
tilt(aa) - tilt(bb)would be a difference of two numbers near5e15andwould manufacture the problem. That branch keeps the direct path, and a test
pins it to the pre-fix expression to
1e-12.Verified two independent ways, neither reusing the package's algebra
The analytic
lam -> 0limit. Aslam -> 0,bb/aa -> 1, sotilt(aa) - tilt(bb) -> 0and the density collapses to a normal: the limitis
-n log sig - n log(2pi)/2 - sum(eps^2)/(2 sig^2), in closed form. Theapproach rate is the real test — the gap must fall like
lam^2:lamThe old form tracks the limit to
1e-4and then turns around.Level-space
log(Phi(aa)/Phi(bb)), valid while neitherPhiunderflows.On 116 grid points in
(lam, mu, sig)where it applies, old and new bothmatch it to better than
1e-9— which is what rules out the rewrite havingbroken the benign regime.
The four samples in the issue reproduce exactly, including its independently
computed Mills-ratio values (
-372.7305,-374.8290,-371.1794).2. The fit could end far below the nested NHN
At
mu = 0the NTN likelihood is NHN's with the same(lambda, sigma), andmu = 0is interior —start_cs()gives NTNlower_bob <- c(rep(.Machine$double.eps, 2), rep(-Inf, n_x_vars + 1)), so onlylambdaandsigmaare bounded. So a maximum below NHN's is a failure, not alimit that is merely approached, unlike TSL's OLS supremum in #28 or the
sigma_vboundary in #56.Fixed on the pattern #45 built for NG and #30 for NNAK: fit NTN's likelihood
with
muheld at 0 from NHN's own starting vector plus five variancesplits, then check the finished fit against that point and polish from it. NTN
is added to the existing
c("NG", "NNAK")list and.nhn_refto the referencesthe loop already walks.
It is an end check, not a start, for the reason that block already records
for NG: as a candidate it moves where the stages go on samples that were never
in trouble.
Measured on 2700 fits
N = 200, seeds 1000001–1000150, all 18y_pcs_*columns, againstmain1edf823:lamfloor and above OLS — impossibleScoring both builds' optima under one correct likelihood: this branch is
higher on 278, lower on 27 (worst 3.78), equal on 2395 —
+12123in totallog-likelihood.
The 13 that remain below NHN are all
y_pcs_ezwithmu = 0andlambdabetween 1320 and 6140 — the flat
sigma_vboundary of #56/A59, where one polishstops short on a ridge. Reported rather than tuned away.
A scoping note, since it is easy to get wrong: 2090 of the 2700 fits are above
OLS at some
lambda, which is entirely legitimate and measures nothing. Only"on the floor and above OLS" is a defect.
Why both halves are in one PR
The cancellation fix alone is a clear net improvement (higher on 124, lower on
73,
+4818total) but it perturbs the optimizer's path, and 73 fits ended lower— worst -30.58, and verified genuine by scoring
main's own optimum underthe corrected likelihood, not an illusion being removed. The nested check
absorbs that tail (73 → 27, worst -3.78). Shipping part 1 alone would knowingly
ship that regression.
mu -> -Infis a supremum — an extra fix measured and then revertedWorth recording because it looked right. The
mu = 0reference sits at exactly0, so
lower.start(differ = 0.5)boxesmuinto[-0.5, 0.5]and it stops onthe edge; three of the four issue samples came back at exactly
mu = -0.5000.Re-centring gained 0.33–1.25 more.
muthen stopped at exactly-2.5(fivepasses of 0.5). Doubling the window gave
-7.5,-15.5,-31.5— every one anexact window boundary.
A continuation profile holding
mufixed at each rung settles it:Gains fall geometrically to a finite limit near
-368.3862withlambdaand
sigmagrowing in proportion to|mu|: a ray to infinity along which thesupremum is approached and never attained — the same kind of object as #56's
boundary. So a widening window is the wrong instrument: there is no point to
walk to, and the fit would stop wherever the cap fell, reporting arbitrarily
large
|mu|with no standard errors. Reverted; what ships is the one-pass check.(An intermediate build with a worse likelihood had wandered onto that same ray
and returned
-368.386084on this sample frommu = -23530. The shippedlikelihood reproduces that value exactly at the same parameter vector — a wrong
likelihood found a right point by accident, and it is the number the profile
converges to.)
Also in this PR
because it joins the loop that performs it. Monotone in the objective.
tests/testthat/test-issue55-ntn-lambda-floor.R: 23 assertions fail onmain, all pass here. Covers the limit and itsO(lam^2)rate, thecannot-beat-OLS bound near the floor, the
mu > 0path being unchanged, theNTN >= NHNfloor, themu = 0identity against a real NHN fit,mu = 0being interior, and every NTN parameter layout (
uhet,muhet, both, nointercept, numeric
start_val) since.nhn_refinserts into avariable-length vector.
test-vcov-bhhh-advice.Rreworked — see the section below.Recorded outside the repository, in the research workspace (so neither the
tarball nor this PR carries them): the long-form derivation in
notes/code_history/sfm.md, three evidence scripts inhorserace/, and gapA61
doneinhorserace/FUNCTIONALITY_GAPS.md— whose status counts werestale and whose own documented recount command was wrong, since
^### [A-Z][0-9]+\.missesA56a,A52a,G2 / I3andH6 / I5. Commandwidened and the numbers rerun.
Cost
An NTN fit goes from 0.084 to 0.264 s (3.2x), because
.nhn_refcosts sixbounded
optim()calls. NG pays four for the same pattern, so it is inprecedent. The full suite is 35 min against a 39 min baseline, i.e. the
cost is not visible there:
FAIL 0 | WARN 43 | SKIP 8 | PASS 5368.The seed budget was measured rather than assumed, and the first measurement was
misleading. Scoring
.nhn_refin isolation, the NHN start alone is already best68% of the time and the five splits add a median of 0.0000 and a max of 0.1466 —
which reads as "drop them". But what matters is the FINISHED fit. Re-fitting the
200 samples that were below NHN on
mainwith a 4-seed build (NHN start +w = 0.3, 0.5, 0.7, matching NG's budget):
Identical on 162. So the splits do earn their place on the subset the check
exists for, and they stay.
One existing test had to be adapted, and why that was its fault not this PR's
test-vcov-bhhh-advice.R(from #54) failed here. Worth reading carefully,because the invariant it guards was never in danger.
It triggers the "bread is undefined" path with a real degenerate NTN fit on
rand = 2. On this branch that fit slides fromlambda = 9.7e5to4.4e6--a log-likelihood move of 9e-5 along the flat ridge it already sat on.
solve(hessian)still fails, every standard error is stillNA, andvcov(type = "bhhh")still fails. But the sandwich bread became invertible,so
vcov(type = "sandwich")returned a matrix instead of erroring and theexpect_s3_class(e, "error")line failed. The trigger stopped triggering; theagreement it then asserts was fine.
That makes the fixture too brittle for the job -- any correct change to NTN can
nudge it. The test now searches
rand = c(2, 4, 5, 12)for a fit whosesandwich path actually errors, asserts the invariant on every one that does, and
ends with
expect_gt(exercised, 0)so a search that found nothing cannot makethe assertions vacuous. It also still requires all four candidates to be the
degenerate kind (
lambda > 1e4, all SEsNA).Verified both ways: 34 assertions pass on this branch, 40 on
main1edf823(more candidates reach the error path there). The redundant first clause of
claims_availablewas dropped -- it was||-ed with the broadergrepl("is defined here")that follows it.Left open deliberately
sigma_vboundary (NTN: warn and flag when sigma_v collapses to the boundary #56/A59 territory)..ne_refholds NG's shape at exactly 1, so NG's end-check box is centred on aconstrained coordinate the same way NTN's was, and may be pinned at
0.5or1.5. Unmeasured; not touched here.y_pcs_tht. There is no such column indata_gen_cs(); the one reproducing its figures isy_pcs_st.🤖 Generated with Claude Code