vcov(): recommend type = "bhhh" only when it is actually available - #54
Merged
Merged
Conversation
When the sandwich/clustered bread is undefined -- the fit carries no
Hessian, or the Hessian is singular -- both failure paths advised
`type = "bhhh"`, on the grounds that BHHH needs no Hessian. True, and not
sufficient: BHHH still inverts crossprod(G), and a fit that reaches these
errors is frequently FLAT in a parameter, which makes the outer product
singular as well. The advice was unconditional, so on those fits it sent
the user to a second error.
Reproduction, sfm(model_name = "NTN") on data_gen_cs(N = 150, rand = 2):
vcov(type = "sandwich") : "... the Hessian is singular ...
type = "bhhh" needs no Hessian and is
defined here."
vcov(type = "bhhh") : "... the outer product of gradients is
singular."
Both paths now probe solve(crossprod(G)) first and recommend BHHH only
when it succeeds; when it does not, they say so and still name the
parameter responsible via .sfa_dead_parameters().
Why that fit is degenerate, verified rather than assumed: NTN has no
interior maximum in lambda on this sample. Profiled by continuation from
the reported solution, the log-likelihood rises monotonically to a finite
limit as lambda -> Inf -- -188.1003 at lambda = 9.7e5, flat at
-188.1001876 from 1e9 through 1e15 -- so the likelihood really is flat in
lambda there. The score column for lambda is 1.24e-10 against 1.15e+07
for the largest, and crossprod(G) has condition number 6.8e32. So
"the likelihood is flat in lambda", which the diagnostic reports, is
independently correct.
Found by measuring test coverage with covr: .sfa_dead_parameters(), which
supplies that parameter name, had NO coverage at all -- 57 of 57
expressions never executed -- so the entire diagnostic path for these two
covariances had never run in the suite. The new test is the first thing
to execute it.
tests/testthat/test-vcov-bhhh-advice.R, 17 assertions. The first block
asserts an INVARIANT rather than a string: the message may claim BHHH is
available only when vcov(type = "bhhh") actually succeeds, both observed
in the same run, so it cannot pass by the wording drifting. The second
block pins the healthy path, since the availability probe is new work on
a hot code path.
Verified to fail on main and pass here: 4 failures against c94721a, 0 on
this branch.
Full suite on this branch, installed build: 0 failures, 5272 passing,
43 warnings, 8 skips, 42.4 min. The warnings and skips are pre-existing;
this test file contributes none of them. The four plm::pdata.frame errors
noted as expected locally did NOT occur in this run.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
davidhbernstein
enabled auto-merge (squash)
October 2, 2026 10:29
davidhbernstein
added a commit
that referenced
this pull request
Oct 2, 2026
NEWS.md only, and the expected conflict: #54 and this branch both insert at the top of `## Bug fixes`. Resolved by keeping BOTH entries with a blank line between, main's merged entry first. Verified after resolving: no conflict markers, both entries present once, #54's bhhh_ok/bhhh_hint code and its test present, and this branch's ntn_sigma_v_at_bound and its test present. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Oct 2, 2026
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.
Found while measuring test coverage, not from a report.
The mechanism
vcov(type = "sandwich")andvcov(type = "clustered")both need the Hessian for their bread. When that is unavailable — the fit carries none, or it is singular — both paths advised the user to switch totype = "bhhh", on the correct observation that BHHH needs no Hessian.That reasoning is true but not sufficient. BHHH still has to invert
crossprod(G), and a fit that reaches these errors is frequently flat in a parameter — which makes the outer product singular too. The advice was issued unconditionally, so on exactly those fits it sent the user to a second error.Reproduction,
sfm(model_name = "NTN")ondata_gen_cs(N = 150, rand = 2):Both paths now probe
solve(crossprod(G))first. BHHH is recommended only when it succeeds; when it does not, the message says so, and either way it still names the parameter responsible through.sfa_dead_parameters().Why that fit is degenerate — verified, not assumed
NTN has no interior maximum in
lambdaon this sample. Profiled by continuation from the reported solution (seeding eachlambdafrom the previous optimum, rather than from a fixed start, which otherwise fails to findsfm()'s own point at largelambda):lambdasfm()'s)Monotone increasing, converging to a finite limit it never attains. So the likelihood genuinely is flat in
lambdaat the reported fit, and the diagnostic's "the likelihood is flat inlambda" is independently correct rather than self-confirming.Supporting numbers at that fit: the score column for
lambdais1.24e-10against1.15e+07for the largest, andcrossprod(G)has condition number6.8e32.How it was found, and why it had survived
Test coverage measured with
covrover the full suite: 87.07% overall, but.sfa_dead_parameters()— the function that supplies the parameter name in these messages — had no coverage at all, 57 of 57 expressions never executed. The entire diagnostic path for these two covariances had never run in the suite, which is why the inconsistency between the advice and reality was never observed. The test added here is the first thing to execute it.Tests
tests/testthat/test-vcov-bhhh-advice.R, 17 assertions, two blocks:vcov(type = "bhhh")actually succeeds — both observed in the same run and compared. It therefore cannot pass because the wording drifted, and cannot fail because the wording was reworded.hessian,bhhh,sandwichandclusteredall still return a full-rankp × pcovariance on an ordinary NHN fit.Evidence
c94721aspecifically: 4 failures there, 0 on this branch.NOT_CRAN=true): 0 failures, 5272 passing, 43 warnings, 8 skips, 42.4 min. The warnings and skips are pre-existing; this test file contributes none of them.plm::pdata.frame: already a pdata.frameerrors documented as expected in local runs did not occur in this run.Scope
A55(PR #17, merged) owns.sfa_dead_parameters()and its logic is unchanged here — this only decides whether the surrounding message recommends BHHH, and passes the diagnostic through on one additional path.🤖 Generated with Claude Code