Repository navigation
Report a negative variance estimate as NA, not as a zero standard error (A64) - #62
Merged
Merged
Conversation
Standard errors were taken as sqrt(pmax(diag(V), 0)) at 21 places, so a
parameter whose estimated variance came back negative -- what an indefinite
Hessian gives -- was reported as st_err 0 and t-val par/0 = Inf: an infinitely
precise estimate for the one direction the covariance machinery has nothing to
say about. The oldest such site dates to CRAN 1.0.4.
Reachable on the first seed tried. psfm("GTRE_FML") on A55's design
(data_gen_p(t = 5, N = 40, rand = 1, ...)) gives three of seven parameters a
negative variance estimate -- x1 -2.83e-06, sigv -3.87e-04, sigh -3.76e-04 --
and reported all three with a standard error of 0 and an infinite t-value,
including the slope x1 = 0.5426.
.sfa_se_from_var() reports a negative or non-finite variance estimate as NA and
takes the square root otherwise; all 21 sites call it. The other four standard
errors on that fit are unchanged to every digit. This matches what the
unclamped entry points already did by taking the square root directly.
Left alone: quantities that are not a parameter's standard error, where a small
negative is rounding on a genuinely near-zero variance and 0 is the right
report -- sfm()'s per-firm posterior spread, the curvature floor, the density
floor; and an exactly-zero variance, since the defect is the clamp
manufacturing zeros out of negatives.
Found while scoping A53 item 5, which claimed sqrt(diag(solve(h))) loses every
negative entry together. R's sqrt() is element-wise, so that item is refuted;
the clamped sibling expression is the real defect. horserace/FUNCTIONALITY_GAPS.md A64.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The end-to-end case asserted expect_gt(sum(v < 0), 0) -- that the GTRE_FML fit on this design really does land on a point whose Hessian inverse has a negative diagonal entry. It does on macOS arm64, where the defect was found (3 of 7 parameters: x1, sigv, sigh), and on none of CI's five platforms, so all five failed on that one assertion and nothing else. The optimizer's landing point is not what the fix changes, so the premise does not belong in an expectation. The invariant that does hold everywhere -- no finite standard error of exactly 0, no infinite t-value -- stays unconditional; the clause about which coordinates lose their standard error is now gated on this platform actually producing a negative estimate, and solve() is wrapped so an exactly singular Hessian skips rather than errors. The helper's own contract, four deterministic tests above, is what pins the fix down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two conflicts, both routine, and one of them worth a note. NEWS.md: both branches added an entry under the top "## Bug fixes". Both kept, A53's first since it is already on main. R/matrix_utils.R: both branches inserted their new helper at the same place -- between .log_nr_g's three-line comment and .log_nr_g itself -- so each had silently orphaned that comment onto its own function, and main carries the same wart from #63. Resolved by putting .sfa_se_from_var() and .se_missing_report() above the comment and re-attaching it to .log_nr_g, which this merge therefore also fixes on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Oct 27, 2026
davidhbernstein
added a commit
that referenced
this pull request
Oct 27, 2026
The test hard-coded the landing point of a single fit on macOS arm64: the names of the two parameters that lose a standard error, and the count 2 of 6. Which point the optimizer stops at, and the sign of a small eigenvalue there, is BLAS-dependent -- exactly what made #62 fail on all five CI platforms a few days ago, and exactly what notes from that round said not to do again. Every expectation is now derived from the fit in front of it: the reported parameters must be the ones whose standard error is non-finite, n_missing must be that count, n_par the length of the vector, and "indefinite" must hold iff a negative eigenvalue was read. The regime is a skip condition, not an assertion, in both directions -- the first test skips where every standard error is finite, since there is then nothing for the report to say, and the false-positive guard skips where the N = 300 fit is not healthy, since its claim is about a healthy fit. expect_silent() is gone too: it demanded total silence from a fit, which is a stronger claim than the test needs. 12 of 12 pass against the branch build and 2 of 4 fail against main, so it still discriminates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
davidhbernstein
added a commit
that referenced
this pull request
Oct 27, 2026
… (A65) (#66) * Name the standard errors the other five entry points could not compute (A65) zsfm(), lcsfm(), copsfm(), ivsfm() and selsfm() each took their standard errors as suppressWarnings(sqrt(diag(solve(opt$hessian)))). On an indefinite Hessian the negative-variance coordinates come back NaN, and suppressWarnings() discarded R's own "NaNs produced" -- the only signal there was. Nothing set a flag, and the convergence code read 0. Reproduced on zsfm("ZISF"), data_gen_cs(N = 25, rand = 8, sig_u = 1, sig_v = .3, cons = .5, beta1 = .5, beta2 = .5, mu = .5, a = 5), 0.5s to fit: eigenvalues 3.48e6 ... -1474, two of six standard errors NaN -- one of them the slope x2, whose true value is 0.5 -- conv 0, no warnings, no flags, and print()/summary() said nothing beyond printing NaN. All five now call .se_missing_report(), the model-agnostic helper #63 added for ttsfm(), on the REPORTED standard errors and names, so copsfm()'s and ivsfm()'s delta-method scales carry through correctly. $se_unresolved is appended to the results list rather than re-listed against its name vector. solve() is wrapped too: the sum(colMeans(hessian)) == 0 test caught only an all-zero Hessian, so a singular but non-zero one errored and discarded a finished fit. The test file fails on main (3 failures: no warning, no flag) and passes 17 of 17 here, and includes a false-positive guard -- the same design at N = 300 must stay silent with no flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Derive A65's expectations from the fit instead of pinning one platform's The test hard-coded the landing point of a single fit on macOS arm64: the names of the two parameters that lose a standard error, and the count 2 of 6. Which point the optimizer stops at, and the sign of a small eigenvalue there, is BLAS-dependent -- exactly what made #62 fail on all five CI platforms a few days ago, and exactly what notes from that round said not to do again. Every expectation is now derived from the fit in front of it: the reported parameters must be the ones whose standard error is non-finite, n_missing must be that count, n_par the length of the vector, and "indefinite" must hold iff a negative eigenvalue was read. The regime is a skip condition, not an assertion, in both directions -- the first test skips where every standard error is finite, since there is then nothing for the report to say, and the false-positive guard skips where the N = 300 fit is not healthy, since its claim is about a healthy fit. expect_silent() is gone too: it demanded total silence from a fit, which is a stronger claim than the test needs. 12 of 12 pass against the branch build and 2 of 4 fail against main, so it still discriminates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * NEWS: conv 0 on the A65 reproduction is machine-specific The entry reported "conv 0, and zero warnings" as the headline. The zero warnings is the defect and holds; conv 0 does not travel. On all five CI platforms the same fit reports convergence 52, so there the convergence code carries a signal and only the missing standard errors are unexplained. On this Mac it reports 0, which is the fully silent case. The CI run also establishes something the local run could not: the warning fired on macOS, ubuntu devel, ubuntu release and windows -- the only failing expectation there was the convergence code -- so the indefinite-Hessian regime is reached on four of the five platforms. ubuntu oldrel-1 does not reach it, which is why the rewritten test skips rather than asserts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Extend A65 to psfm(), which has a seventh site and a third spelling psfm() carries the same two defects at seven Hessian-diagonal sites, covering GTRE and TRE (which share one), GTRE_Z, TRE_Z, GTRE_FML, TFE_WMLE, TFE and FD. Four of the seven left the inverse unguarded, so a singular but non-zero Hessian discarded a finished PANEL fit -- the most expensive kind this package produces. Their guard tests any(hessian == 0), which catches a zero ENTRY rather than singularity. The count recorded two days ago was six, and it was wrong. FD spells the same quantity sqrt(diag(qr.solve(opt$hessian))) -- qr.solve, and unclamped -- which makes it invisible to A64's sweep, to A65's first pass, and to the .sfa_se_from_var(diag(solve(...))) pattern that found the other six. It surfaced only because a replace-all asserted 4 occurrences and got 5; the sweep was then redone on $hessian itself, spelling-agnostically, which is what should have happened first. Deliberately excluded after that sweep: psfm()'s two other Hessian inversions (the sandwich H_inv, the delta-method vc), which already guard solve() and take their standard errors through a Jacobian, so the report's eigenvalue narrative does not map onto them as directly. $se_unresolved is appended at eight assemblies -- GTRE and TRE share a site but keep separate results lists -- and the insertion was scoped to each enclosing if (model_name ...) block so it could not leak into GTRE_SEQ1/SSFE/PL80/GTRE_SEQ2, where .sm does not exist. Two instances came out of the existing suite, which is why the local warning count moved: * psfm("FD") at test-psfm.R:23 is GENUINE -- 3 of 5 standard errors missing, smallest eigenvalue -0.00119 against a largest of 17400. So the site every sweep missed is reachable in the package's own tests, and had been reporting three unexplained NAs there all along. * lcsfm("LCM_CN") at test-a52-opt-shape.R:91 was a FALSE POSITIVE of mine. That call passes optHessian = FALSE, so there is no Hessian by request; LCM_CN is the one branch whose standard-error test does not mention optHessian, and so the one place the guard was missing. Now guarded. Test uses A64's own design, psfm("GTRE_FML") on data_gen_p(t = 5, N = 40, rand = 1, ...) -- 3 seconds, three of seven parameters with a negative variance estimate -- gated on the regime from the start, since A64's end-to-end case had to be gated after all five CI platforms disagreed with this Mac. Full local suite on the final code: pass 5556, fail 0, warn 49, skip 9, against a 48-warning baseline -- the one addition being the genuine FD report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- 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.
A negative variance estimate was reported as a standard error of exactly zero
Standard errors were taken as
sqrt(pmax(diag(V), 0))at 21 places inR/.The clamp means a parameter whose estimated variance came back negative --
which is exactly what an indefinite Hessian gives -- was reported as
st_err = 0andt-val = par/0 = Inf: an infinitely precise estimate for theone direction the covariance machinery has nothing to say about. The oldest of
those sites dates to CRAN 1.0.4 (2026-02-04).
Reachable on the first seed tried, on this machine
psfm(model_name = "GTRE_FML")on A55's own design --data_gen_p(t = 5, N = 40, rand = 1, sig_u = 1, sig_v = 0.3, sig_r = 0.2, sig_h = 0.4, cons = 0.5, beta1 = 0.5, beta2 = 0.5, eta = 0.1), 3.1 seconds to fit:Three of seven parameters, one an ordinary slope (
x1 = 0.5426), reported witha standard error of zero. Over seeds 1-10 of that design: 1 shows this, 4
return all-NA standard errors because
solve()fails outright (alreadyhandled), 5 are ordinary. It is not rare arithmetic -- it is what an indefinite
Hessian always produces.
The change
.sfa_se_from_var()(R/matrix_utils.R) reports a negative or non-finitevariance estimate as
NAand takes the square root otherwise. All 21 sitescall it:
psfm(10),panel_classicalanduhet_test(2 each), and one eachin
sfm,esfm,het,kss,selsfm_kts,robust_divergenceandmatrix_utils. On seed 1 the three negative coordinates becomeNA, theother four standard errors are unchanged to every digit, and no
Inft-value survives. This is what the unclamped entry points (
ttsfm,zsfm,lcsfm,copsfm,ivsfm,selsfm) already did by taking the square root ofthe negative value directly.
vcov()'s singular-Hessian fallback builds a diagonal from the reportedstandard errors (
R/sfareg_methods.R:209), so it now carriesNAfor thoseparameters instead of a zero variance. Its
!all(is.na(...))guard isunaffected.
Deliberately left alone
sfm()'s per-firmposterior spread
sd_exp_u_hatissqrt(pmax(E[exp(-2u)] - E[exp(-u)]^2, 0))by quadrature, where a small negative is rounding on a genuinely near-zero
variance and
0is the right report; changing it would blank the column forwell-behaved firms. Likewise the
1e-3curvature floor and thedouble.xmindensity floor, which are not variances.0. The defect is theclamp manufacturing zeros out of negatives.
sigrin the table above, at5.24e-08with variance4e-18, standarderror
2e-09and t-value 26.2 -- a collapsed parameter reported ascomfortably significant. That is A55's open territory (name the parameter
that died), it survives this change untouched, and it is arguably more
misleading than the
Infbecause it looks like an ordinary number.Tests
tests/testthat/test-a64-negative-variance-se.R, 14 assertions: the helper'scontract, that a t-value built on it is
NArather thanInf, and theend-to-end
GTRE_FMLfit above. Onmainthe file gives 7 failures(4 errors for the missing helper, plus the three end-to-end assertions:
se == 0count, infinite-t count, and NA on exactly the negativecoordinates); on this branch 14 pass, 0 fail on macOS.
The end-to-end case is platform-dependent, and the first CI run proved it.
It originally asserted
expect_gt(sum(v < 0), 0)-- that this design reallydoes reach a negative variance estimate. It does here (3 of 7 parameters) and
on none of CI's five platforms, so all five jobs failed on that one
assertion and nothing else (
FAIL 1each, 44m-2h06m) while the fix itself wascorrect. Which point the 3-stage minimiser lands on, and the sign of a small
eigenvalue there, is BLAS-dependent arithmetic; the fixed seed makes the data
identical everywhere, not the fit. That premise is now gated on
skip_if_not(any(v < 0), ...)with thesolve()wrapped intryCatch, so theclause skips off this Mac. The invariant stays unconditional -- no finite
standard error of exactly
0, no infinite t-value -- and the helper's fourdeterministic unit tests, whose input is constructed rather than optimised to,
carry the proof of the fix.
Full local suite against the merged branch build, after #63 landed and this
branch took
main(macOS,load_package = "installed",NOT_CRAN=true,TESTTHAT_CPUS=2): pass 5531, fail 0, warn 48, skip 9. For the record, CI'swarning counts on the pre-merge branch ranged 182-225 across the five platforms
against identical code, which is why 48 is reported rather than compared.
The merge with
main, and one thing it fixesNEWS.mdconflicted as expected -- both entries kept, A53's first since it isalready on
main.R/matrix_utils.Rconflicted because both branches insertedtheir helper at the same place: between
.log_nr_g's three-line comment and.log_nr_gitself, so each had silently orphaned that comment onto its ownfunction, and
maincarries the same wart from #63. The merge puts.sfa_se_from_var()and.se_missing_report()above the comment andre-attaches it, so landing this also fixes that on
main.There is no same-invocation
mainbaseline for the warning count this round,so 48 is reported rather than compared; the change cannot add warnings, since
.sfa_se_from_var()emits none andsqrt()ofNAis silent wheresqrt()of a negative was not.Found while scoping A53 item 5, which claimed
sqrt(diag(solve(h)))"destroysevery one of them together" and prescribed per-parameter reporting. R's
sqrt()is already element-wise (sqrt(c(4, -1, 9))is2 NaN 3), so thatitem is refuted and recorded as such; the clamped sibling expression is the
real defect.
🤖 Generated with Claude Code