Repository navigation
Report a boundary sigma_u as suspect when the residual skew says inefficiency is present - #59
Merged
Merged
Conversation
…is there sfm() sets $sigma_u_at_bound when the fitted one-sided scale reaches zero, and the two cases that reach it read in OPPOSITE directions: - WRONG skew (m3 >= 0): the boundary IS the MLE, the Type I failure of Olson, Schmidt and Waldman (1980). Read as no evidence of inefficiency. - CORRECT skew: the data say inefficiency is present, so the boundary should not be the answer and the fit is suspect. Only the first was handled. The fit-time warning was guarded on `wrong_skew && at_bound`, leaving the second flagged and silent. The printed NOTE was worse: it dropped the clause naming the skew but kept "Under wrong skewness this is the correct MLE, not a failure (Waldman 1982)" in both cases, so the suspect fit was shown the sound fit's exculpation -- in the output that survives saveRDS() and a fresh session, which is what the comment above .sfa_report_boundary() says people actually read. Measured on NW, y_pcs_w, N = 150 (gap A60): seeds 1, 5 and 7 all give conv 52, no standard errors, at_bound TRUE, wrong_skew FALSE, and residual m3 of -1585.5, -469.3, -4007.9. Two are not stationary -- moving sigma_v alone gains 0.4771 and 0.0452 -- and seed 1 has a point 6.5743 better at the same sigma_u. Both paths now branch on the skew. The new warning names the residual moment, reports a non-zero convergence code when there is one, says the fit may not be a maximum, and says the wrong-skew reading does not apply. The sound case keeps its message unchanged. The guard is model-generic, not NW-specific, so this covers any sfm() model reporting sigu or lambda. Tests drive both reporting paths directly rather than through a fit: the models that reach this state sit on a flat ridge where the optimizer's stopping point is not stable across platforms, which is what made the vcov fixture and issue #55's tests fail on CI but not locally. The printed-NOTE assertion fails on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tHN warns at sfm.R:1612 that heavy-tailed noise absorbing the entire one-sided component is a known property of the model, not necessarily a numerical failure -- and its row names (sigv|sigu|nu) put it through the generic sigma_u block as well. So the previous commit would have printed "this fit may not be a maximum" and "a known property of tHN, not necessarily a numerical failure" for the same fit, on the same console. The warning path excludes tHN by name. The print path does too, and prints tHN's own reading in its place -- including when only the generic flag fired, since the two use different thresholds (tHN: sig_u/sqrt(sig_u^2+sig_v^2) < 1e-3; generic: sigu/sd(resid) < 1e-2), so the generic one can fire alone. Previously that case printed the Waldman sentence, which was wrong for tHN twice over. Tests cover both tHN threshold combinations, tHN under wrong skew (which keeps the Waldman reading unchanged), that exactly one NOTE is emitted rather than two contradicting ones, and that an object with no model_name does not fall into the tHN branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The explanation now lives in notes/code_history/sfm.md, which is where the tHN exclusion rationale and the print-path finding belong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
davidhbernstein
enabled auto-merge (squash)
October 8, 2026 15:29
CI failed on all five platforms with one assertion, FAIL 1 | PASS 5449.
expect_match(w, "does NOT") was checking the PRINT path's phrasing ("The
Waldman (1982) wrong-skew reading does NOT apply here") against the
WARNING's text, which denies the same reading in different words. A copy
error between the two messages; nothing in the package was wrong.
Now asserts what the warning actually says, and keeps the invariant the
assertion was for -- that it does not hand over the wrong-skew
exculpation:
"Do NOT read it as no evidence of inefficiency"
"applies only under wrong skew"
Verified against a build pinned with git archive, outside Dropbox: 43
assertions pass on the branch, and the file fails on unmodified main with
10 failures. On main the correct-skew object prints "Under wrong skewness
this is the correct MLE, not a failure (Waldman 1982)", which is the
defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Oct 16, 2026
davidhbernstein
added a commit
that referenced
this pull request
Oct 23, 2026
…o date (#61) Issue #3 asks for a troubleshooting section for the optimizer and boundary states. The README has had one since an earlier pass, but it is the landing page rather than the help system, and its first bullet still gave only half of the wrong-skew story -- PR #59 split a collapsed sigma_u into two opposite readings and sfm.Rd and NEWS were updated while the README was not. A reader following it would have taken a suspect fit for the Waldman case. man/sfa-troubleshooting.Rd is the help-system version, built as a sibling of sfa-wrongskew.Rd and in the same shape: symptom first, remedy after. It covers what the README does not -- the two readings of a boundary sigma_u, NTN's sigma_v collapse and why every standard error is NA there, tHN's own boundary and its multistart diagnostic, convergence codes 99, 51 and 52 and what sfa_diagnostics() will and will not call benign, vcov(type = "bhhh") where the Hessian is singular, why sandwich and clustered need keep_objective, simulation noise in the simulated-ML models, the psfm estimators that maximise no likelihood, the factor of two between sfm's and psfm's variance-determinant links, and the reparameterised row names. Every claim was read off the code rather than the docs: the 99 path and its message in opts.R, the 51/52 labels and the benign/unverified verdict in diagnostics.R, the bhhh hint in sfareg_methods.R, SSFE carrying no $opt in psfm.R, and NU reporting theta rather than sigu in starting.values.R. Docs only, so no NEWS entry. 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.
Closes gap A60.
The defect
sfm()sets$sigma_u_at_boundwhen the fitted one-sided scale reaches thezero boundary. Two cases reach it, and they read in opposite directions:
m3>= 0(wrong skew)< 0(correct skew)Only the first was handled.
The warning was guarded on the conjunction
isTRUE(ws$wrong_skew) && isTRUE(ws$at_bound), so the second case was flagged and silent. That guardis correct for the message it protects --
.warn_wrong_skew_boundary()explainsthe Olson-Schmidt-Waldman case, which needs
m3 >= 0-- but it left theopposite case with no message at all.
print()andsummary()were worse..sfa_report_boundary()dropped theclause naming the skew but kept the sentence after it:
So the suspect fit was shown the sound fit's exculpation. That matters more than
the missing warning, and the comment above that very function says why: a
warning "does not survive
saveRDS(), a fresh session, or a loop thatsuppressed warnings"; the printed table is "what people look at afterwards".
Measured
sfm(model_name = "NW"),y_pcs_w,N = 150, seeds 1-8. Three fits land here,all profiled with the package's own
$objective(keep_objective = TRUE), not atranscription -- NW is simulated ML, so a transcription would use different
Halton draws and the comparison would be meaningless.
convm3sigma_valone$sigma_u_at_bound$wrong_skewTwo of the three are not even stationary: holding every other parameter at
its fitted value and moving
sigma_valone improves the log-likelihood. On seed1 a point 6.5743 better exists at the same
sigma_u(sigma_v3.1328 -> 2.8300,
k0.1344 -> 0.1314), verified by evaluating$objectiveatthe full candidate vector. Seed 7 is stationary in that coordinate and has not
been shown non-stationary in another, so the non-stationarity claim is 2 of 3.
So a user saw a boundary flag, no standard errors, and a printed note calling it
the correct MLE -- on a fit that returned
ABNORMAL_TERMINATION_IN_LNSRCHandwas not a maximum.
The fix
Both reporting paths now branch on the skew. The suspect case gets its own
message, which names the residual moment, reports a non-zero convergence code
when there is one, says the fit may not be a maximum, and states explicitly that
the wrong-skew reading does not apply. The sound case keeps its message
unchanged, verbatim.
The guard is model-generic, not NW-specific -- it fires for any
sfm()modelreporting
siguorlambda-- so this covers the class, not just the three NWfits that exposed it.
One exclusion, which is the subtle part
tHNis excluded and keeps its own reading. It already warns atsfm.R:1612that heavy-tailed noise absorbing the entire one-sided component isa known property of the model rather than a numerical failure, and its row names
(
sigv|sigu|nu) put it through this same generic block. Without the exclusionone tHN fit would print both "this fit may not be a maximum" and "a known
property of tHN, not necessarily a numerical failure".
The print path excludes it too, and substitutes tHN's own reading -- including
when only the generic flag fired, because the two thresholds differ
(tHN:
sig_u/sqrt(sig_u^2+sig_v^2) < 1e-3; generic:sigu/sd(resid) < 1e-2),so the generic one can fire alone. That case previously printed the Waldman
sentence, which was wrong for tHN twice over.
Tests
tests/testthat/test-a60-boundary-correct-skew.R. The printed-NOTE assertionfails on
main: the old text contained "correct MLE" and "not a failure" inthe correct-skew case, and the test asserts their absence.
The tests drive the two reporting paths directly rather than through a fit.
That is deliberate, and it is the lesson from the last two PRs: the models that
reach this state sit on a flat ridge where the optimizer's stopping point is not
a stable property across platforms. #54's vcov fixture slid
9e-5along such aridge and stopped triggering; #57's tolerance test passed on macOS and failed on
four other platforms. A fixture chosen for being degenerate is a fixture chosen
on a flat ridge. The reporting logic here is a deterministic function of three
recorded fields, so it is tested as one.
Coverage: both skew branches, both tHN threshold combinations, tHN under wrong
skew, exactly-one-NOTE rather than two contradicting ones, the interior and
NAcases staying silent, the convergence clause appearing at
conv 52and droppingat
0andNA, and the two warnings staying textually distinguishable so acaller filtering on one does not catch the other.
Still to measure, and not claimed here: how often the new warning fires on
sound fits across models and sample sizes, a fast non-simulated-ML fixture for
an integration test, and whether NLN (the other simulated-ML model on this code
path) behaves the same. A60 measured only
y_pcs_watN = 150. That sweep isqueued on the Windows box; this PR deliberately does not assert anything about
it.
🤖 Generated with Claude Code