ivsfm: refuse uhet for C2SLS instead of silently ignoring it - #46
Merged
Merged
Conversation
C2SLS is 2SLS with a homoskedastic moment correction and has no delta to estimate. It accepted `uhet` and returned the homoskedastic fit unchanged -- same coefficients, same jlms, no delta_ rows -- so asking for the APS (2017) model returned the 2016 one without a word. Now an error pointing to IVLIML or IVCF. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014oJL17ewiahA8pWbemnqNp
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
2 participants
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.
ivsfm(model_name = "C2SLS", uhet = ~q)returned the homoskedastic 2016 fit as ifuhethad not been given. This PR makes it an error.Mechanism
C2SLS (APS 2016, Section 4.1) does three things:
None of these steps involves environmental variables. The C2SLS branch of
ivsfm()never readsQ, theuhetdesign matrix. Butuhetwas accepted for everymodel_name. The only places that readQare the IVLIML/IVCF likelihood and its JLMS. So a user who asked C2SLS for the APS (2017) model got the 2016 one, with no warning and nodelta_rows.Measured
On main (
1c7d78e) I ran a sample with n = 600 and σ_u,i = exp(0.5 q_i). Theoutmatrices with and withoutuhet = ~qwere identical, andidentical(a$jlms, b$jlms)wasTRUE.Fix
Supplying
uhetwith"C2SLS"is now an error that points to"IVLIML"or"IVCF". Those two estimate δ, and an existing test checks that they recover it.man/ivsfm.Rdnow says this in theuhetargument.\usageis unchanged, andtools::checkRdis clean.Tests
test-ivsfm.Radds "C2SLS refuses uhet rather than ignoring it". It fails on main and passes here.I ran
test-ivsfm*.Randtest-endogeneity-test.Rwithtest_dir(load_package = "installed")on R 4.3.3, on main and on this branch. The results are the same apart from the new test.One failure appears on both: "ivsfm rejects malformed calls" (line 189). With
instruments = ~1, R 4.3'sstats::reformulate(character(0))errors before the "excluded instrument" message is reached. The package declaresR (>= 4.0.0), so this is a real defect on older R, but it is not this PR's. I'll handle it separately.NEWS entry under ## Bug fixes.
🤖 Generated with Claude Code
https://claude.ai/code/session_014oJL17ewiahA8pWbemnqNp
Generated by Claude Code