iframe-proxy

igerber · 2026-06-28T22:45:28Z

Summary

Route the structural (non-covariate) design-Gram bread inversions through the shared
scale-invariant _rank_guarded_inv (diff_diff/linalg.py) — the same generalized inverse
already used for the covariate influence-function SEs (sibling of fix(staggered): scale-equilibrate CS / StaggeredTripleDiff covariate OR fits #570). Affected: ContinuousDiD
ACRT-variance (Psi'WPsi), TwoStageDiD Stage-2 (X_2'WX_2, both the analytical
two_stage.py and the multiplier-bootstrap two_stage_bootstrap.py surfaces), SpilloverDiD
Wave D (A_22), and the Conley spatial-HAC variance (X'WX).
The bug: np.linalg.inv/solve raise only on an exactly singular matrix, so a
near-singular internal Gram returned a garbage inverse (~1e13) straight into the SE.
(ContinuousDiD's exact-singular fallback was additionally a silent minimum-norm pinv; Conley
raised ValueError.) These bases are internal — users cannot perturb them with covariates=
(distinct from the already-fixed covariate path). _rank_guarded_inv truncates redundant
directions on the equilibrated Gram → a finite SE on the identified subspace (the well-conditioned
near-collinear limit, not minimum-norm; NaN only at rank 0) and emits a UserWarning when a
direction is dropped.
Behavior change: a rank-deficient Conley design no longer raises — it rank-reduces with a
warning, uniform with the other structural breads and the covariate convention. Well-conditioned
designs are unchanged (the fast path is np.linalg.solve(A, I); R-parity preserved). conley.py
imports _rank_guarded_inv lazily because linalg imports conley one-way (a top-level import
would be circular).
Excluded after premise verification (the TODO row over-listed 6 sites; both closed out in
TODO.md): HeterogeneousAdoptionDiD's Zd'WX is a non-symmetric IV bread, so the
symmetric-PSD _rank_guarded_inv is methodologically inapplicable; ImputationDiD's vcov is
already rank-guarded upstream via solve_ols (its only raw inverse is a Wald F-test statistic
with a safe NaN fallback, not a sandwich bread).

Methodology references (required if estimator / math changes)

Method name(s): ContinuousDiD (CGBS 2024), TwoStageDiD (Gardner 2022), SpilloverDiD
(Butts 2021/2023), ConleySpatialHAC (Conley 1999) — the influence-function / sandwich
variance (SE) path only.
Paper / source link(s): docs/methodology/REGISTRY.md — a new - **Note (rank-guarded ...)** in
each of the four estimator sections, cross-referencing the CallawaySantAnna "rank-guarded IF
standard errors" Note (the established column-drop generalized-inverse semantics).
Any intentional deviations from the source (and why): None new. This extends the existing
column-drop generalized-inverse convention (already used for CS / TripleDifference /
StaggeredTripleDifference covariate IF SEs) to the structural breads. The Conley raise→rank-reduce
is the documented, intentional behavior change. No change to estimands, identifying assumptions,
point estimates, or the well-conditioned SE path.

Validation

Tests added/updated: tests/test_conley_vcov.py (direct-call: singular Gram → finite rank-reduced
SE, not a raise; rank-0 → NaN; column-drop == near-collinear-limit anchor),
tests/test_continuous_did.py + tests/test_spillover.py (rank-drop warning + finite SE via the
_rank_guarded_inv seam), tests/test_two_stage.py (the two Stage-2 bread-warning tests
re-pointed from the np.linalg.solve seam — which now collides with _rank_guarded_inv's
internal solve — to the _rank_guarded_inv seam, asserting the rank-reduce message).
Evidence: 748 affected-suite tests pass (test_continuous_did / test_two_stage /
test_spillover / test_conley_vcov / test_methodology_conley / test_methodology_two_stage);
mypy 0 new errors vs baseline; source edits black/ruff-clean. The Conley sandwich
bread_inv @ meat @ bread_inv is algebraically identical to the prior two symmetric solves
(verified). Well-conditioned no-regression is covered by the existing suites (the fast path is the
unchanged np.linalg.solve(A, I)).

Security / privacy

Confirm no secrets/PII in this PR: Yes

🤖 Generated with Claude Code

github-actions · 2026-06-28T22:49:00Z

Overall Assessment

⚠️ Needs changes — one unmitigated P1 finding: rank-dropped named coefficients can surface with se=0.0 instead of NaN.

Executive Summary

The registry documents the rank-guard convention for all four affected methods, so the rank-reduction behavior itself is not a methodology defect.
The changed coefficient-aligned variance paths lose the rank guard’s dropped-column identity and can report zero SE for undefined coefficients.
ContinuousDiD’s transformed ACRT/dose-response use appears consistent with the documented reduced-subspace convention.
The new tests mostly patch the warning seam and do not exercise the actual rank-reduced inverse behavior in estimator outputs.

Methodology

Severity: P1
Impact: _rank_guarded_inv zero-fills dropped rows/columns but returns only (inverse, n_dropped, rank). TwoStageDiD, SpilloverDiD, and Conley coefficient-level paths then compute SEs from those zero diagonals, so a dropped named effect can be reported with se=0.0 even though the coefficient direction is unidentified. safe_inference() will NaN the t/p/CI, but the SE itself is incorrect statistical output.
Locations: diff_diff/linalg.py:L315-L434, diff_diff/two_stage.py:L3186-L3201, diff_diff/two_stage.py:L2451, diff_diff/two_stage.py:L2650-L2652, diff_diff/two_stage.py:L2761-L2764, diff_diff/spillover.py:L3376-L3397, diff_diff/spillover.py:L3490-L3508, diff_diff/conley.py:L1119-L1131, diff_diff/linalg.py:L3886-L3908.
Concrete fix: Have _rank_guarded_inv expose kept/dropped column indices or a mask. In coefficient-aligned callers, re-expand the vcov with NaN rows/cols for dropped named coefficients before SE extraction. Keep finite reduced-subspace variance only for identified linear combinations.
Severity: P3
Impact: The Conley exact-singular behavior change from ValueError to rank-reduction is documented in REGISTRY.md, so it is informational, not a blocker.
Concrete fix: None required.

Code Quality

Severity: None
Impact: No additional code-quality issues beyond the dropped-column API gap above.
Concrete fix: None.

Performance

Severity: None
Impact: No material performance regression identified in the changed paths; bread dimensions are small structural designs.
Concrete fix: None.

Maintainability

Severity: P2
Impact: Returning only n_dropped/rank from _rank_guarded_inv makes correct downstream coefficient mapping impossible for callers that expose named coefficients.
Concrete fix: Return a small result object or tuple including kept/dropped indices, and update all new structural-bread callers to use it.

Tech Debt

Severity: None
Impact: The removed TODO item and the two new “Deferred / Documented” exclusions for HAD and ImputationDiD are properly tracked.
Concrete fix: None.

Security

Severity: None
Impact: No secrets, injection surfaces, or security-sensitive changes identified.
Concrete fix: None.

Documentation/Tests

Severity: P2
Impact: The new estimator tests for ContinuousDiD, SpilloverDiD, and TwoStageDiD fake n_dropped=1 while returning the full inverse, so they assert warnings but not actual rank-reduced output. This misses the zero-SE dropped-coefficient issue.
Locations: tests/test_continuous_did.py:L1546-L1565, tests/test_spillover.py:L8843-L8857, tests/test_two_stage.py:L1473-L1499, tests/test_two_stage.py:L1512-L1535.
Concrete fix: Add tests with actual rank-dropped breads for TwoStage event/group effects, Spillover ring/event coefficients, and Conley coefficient inference. Assert dropped named coefficients have se, t_stat, p_value, and CI all NaN, while retained identified effects remain finite.

Path to Approval

Extend _rank_guarded_inv to return dropped/kept column indices.
Use that mask in TwoStageDiD, SpilloverDiD, and Conley coefficient-aligned vcov paths to set dropped named coefficient rows/cols to NaN.
Update REGISTRY.md wording so “NaN only at rank 0” does not imply dropped named coefficients may report zero SE.
Add real rank-drop tests covering dropped named coefficient SEs, not only warning emission.

github-actions · 2026-06-28T23:12:17Z

🔁 AI review rerun (requested by @igerber)

Head SHA: 013470b76b510ede522493334fc7e39ea3f14cb1

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings. The prior P1 zero-SE issue appears resolved.

Executive Summary

_rank_guarded_inv() now exposes a dropped-coordinate mask, addressing the prior API gap.
Analytical coefficient-level paths now NaN dropped rows/cols in final VCVs for TwoStageDiD, SpilloverDiD, and Conley.
The structural rank-guard behavior is documented in REGISTRY.md for all affected methods, so the Conley raise-to-rank-reduce behavior is an accepted documented deviation.
Bootstrap TwoStageDiD now NaNs dropped bootstrap coefficient columns; current behavior is consistent with the helper’s zero-fill contract.
Remaining concern is test depth: TwoStage/Spillover estimator tests still mostly mock the rank-drop seam rather than exercising real dropped structural coefficients.

Methodology

Severity: P3
Impact: Affected methods are ContinuousDiD (CGBS 2024), TwoStageDiD (Gardner 2022), SpilloverDiD (Butts/Gardner/Conley synthesis), and ConleySpatialHAC (Conley 1999). The rank-guarded structural bread behavior and Conley behavior change are explicitly documented with Note labels, so these are not methodology defects.
Locations: docs/methodology/REGISTRY.md:L864, docs/methodology/REGISTRY.md:L1430, docs/methodology/REGISTRY.md:L3675, docs/methodology/REGISTRY.md:L3912
Concrete fix: None required.

Severity: None
Impact: Prior P1 resolved. _rank_guarded_inv() returns dropped masks, and coefficient-aligned VCV paths set dropped coefficient rows/cols to NaN before SE extraction.
Locations: diff_diff/linalg.py:L315-L468, diff_diff/two_stage.py:L3186-L3208, diff_diff/spillover.py:L3376-L3391, diff_diff/conley.py:L1119-L1138
Concrete fix: None required.

Code Quality

Severity: None
Impact: No code-quality blocker found in the changed lines.
Concrete fix: None required.

Performance

Severity: None
Impact: No material performance regression identified; the guarded inversions are on small structural bread matrices, and the full-rank fast path still uses np.linalg.solve(A, I).
Concrete fix: None required.

Maintainability

Severity: P3
Impact: TwoStage bootstrap infers dropped coefficients from all-zero bread columns instead of threading the dropped mask returned by _rank_guarded_inv(). This is currently correct because the helper’s documented rank-deficient representation zero-fills dropped rows/cols, but it is more brittle than using the explicit mask.
Locations: diff_diff/two_stage_bootstrap.py:L284, diff_diff/two_stage_bootstrap.py:L574-L580, diff_diff/two_stage_bootstrap.py:L640-L643
Concrete fix: Optionally have _compute_cluster_S_scores() request and return the dropped mask, then use it directly for bootstrap NaN propagation.

Tech Debt

Severity: P3
Impact: The excluded HAD and ImputationDiD structural-rank-guard items are documented in TODO.md, so they are tracked deferred work rather than blockers.
Locations: TODO.md:L152-L153
Concrete fix: None required.

Security

Severity: None
Impact: No secrets or security-sensitive changes found in the reviewed diff.
Concrete fix: None required.

Documentation/Tests

Severity: P2
Impact: The prior P2 test-depth gap is only partly addressed. Conley has a real direct-call rank-drop test, and _rank_guarded_inv() has mask tests, but TwoStage/Spillover estimator tests still force n_dropped=1 with an empty dropped mask, so they verify warning emission rather than actual estimator-level NaN propagation for dropped structural coefficients. This is not a blocker, but it leaves the key regression mostly covered indirectly.
Locations: tests/test_two_stage.py:L1475-L1484, tests/test_two_stage.py:L1519-L1521, tests/test_spillover.py:L8845-L8851, tests/test_conley_vcov.py:L3697-L3707, tests/test_linalg.py:L2225-L2253
Concrete fix: Add real or minimally patched rank-drop tests where _rank_guarded_inv() returns a non-empty dropped mask and assert TwoStage event/group plus Spillover ring outputs have se, t_stat, p_value, and CI all NaN.

Tests were not run; this was a static PR review.

github-actions · 2026-06-28T23:26:19Z

🔁 AI review rerun (requested by @igerber)

Head SHA: 555e44ca8514ffeb78d36309ba3bb312dea2f9d9

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

Affected methods: ContinuousDiD, TwoStageDiD, SpilloverDiD, and Conley spatial-HAC variance.
The rank-guard behavior and Conley raise-to-rank-reduce behavior are documented in REGISTRY.md, so they are not methodology defects.
The previous P1 zero-SE issue appears addressed: dropped named coefficients now get NaN rows/cols or NaN bootstrap columns before inference.
TwoStage bootstrap now threads the explicit dropped mask, resolving the prior maintainability concern.
Remaining issues are P2/P3 only: test depth and one stale Spillover comment.

Methodology

Severity: P3 informational
Impact: Structural bread rank-reduction is a documented deviation/behavior note for all affected estimators. Conley’s exact-singular behavior change from ValueError to warning + rank reduction is explicitly documented.
Locations: docs/methodology/REGISTRY.md:L864, docs/methodology/REGISTRY.md:L1430, docs/methodology/REGISTRY.md:L3675, docs/methodology/REGISTRY.md:L3912
Concrete fix: None required.

Severity: None
Impact: Prior NaN/zero-SE methodology concern is resolved in the changed paths: analytical TwoStage NaNs dropped VCV rows/cols, TwoStage bootstrap NaNs dropped coefficient columns, Spillover NaNs dropped kept-submatrix rows/cols before re-inflation, and Conley NaNs dropped coefficient rows/cols.
Locations: diff_diff/two_stage.py:L3186-L3208, diff_diff/two_stage_bootstrap.py:L562-L608, diff_diff/two_stage_bootstrap.py:L629-L667, diff_diff/spillover.py:L3376-L3403, diff_diff/conley.py:L1119-L1138
Concrete fix: None required.

Code Quality

Severity: P3
Impact: A nearby Spillover comment still describes the bread sandwich as using np.linalg.solve with dense lstsq fallback, but the code now uses _rank_guarded_inv. This is not behavioral, but it is misleading maintenance context.
Location: diff_diff/spillover.py:L3360-L3364
Concrete fix: Update the comment to describe _rank_guarded_inv and rank-reduction.

Performance

Severity: None
Impact: No material regression found. The full-rank fast path in _rank_guarded_inv() still calls np.linalg.solve(A, I), and the guarded matrices are small structural breads.
Concrete fix: None required.

Maintainability

Severity: None
Impact: Prior P3 concern is resolved: TwoStage bootstrap now returns and uses the explicit dropped mask instead of inferring dropped coefficients from zero-filled bread columns.
Locations: diff_diff/two_stage_bootstrap.py:L140-L155, diff_diff/two_stage_bootstrap.py:L288-L302, diff_diff/two_stage_bootstrap.py:L584, diff_diff/two_stage_bootstrap.py:L647
Concrete fix: None required.

Tech Debt

Severity: P3 informational
Impact: The excluded HeterogeneousAdoptionDiD and ImputationDiD sites are tracked in TODO.md with rationale, so they are not blockers under the deferred-work rules.
Location: TODO.md:L152-L153
Concrete fix: None required.

Security

Severity: None
Impact: No secrets or security-sensitive changes found in the reviewed diff.
Concrete fix: None required.

Documentation/Tests

Severity: P2
Impact: Test coverage improved, especially Conley direct-call and TwoStage analytical event-study dropped-mask propagation. Remaining coverage gap: Spillover still only forces a warning with an empty dropped mask, and TwoStage bootstrap/group dropped-mask propagation is not directly asserted at estimator level. This is not a blocker, but it leaves the key regression partly indirect.
Locations: tests/test_two_stage.py:L1546-L1596, tests/test_spillover.py:L8837-L8862, tests/test_conley_vcov.py:L3691-L3734
Concrete fix: Add tests where _rank_guarded_inv() returns a non-empty dropped mask for Spillover ring output and TwoStage bootstrap/group output, then assert se, t_stat, p_value, and CI are all NaN.

Tests were not run: pytest is not installed in this environment.

ContinuousDiD (ACRT-variance Psi'WPsi), TwoStageDiD (Stage-2 X_2'WX_2, analytical + multiplier-bootstrap surfaces), SpilloverDiD (Wave D A_22), and the Conley spatial-HAC variance (X'WX) inverted their internal design-Gram bread on a LinAlgError-only fallback: np.linalg.inv/solve raise only on an exactly-singular matrix, so a near-singular Gram returned a garbage inverse (~1e13) straight into the SE (ContinuousDiD's exact-singular fallback was a silent minimum-norm pinv; Conley raised ValueError). Route all four through the shared _rank_guarded_inv (the generalized inverse already used for the covariate IF SEs): it truncates redundant directions on the equilibrated Gram -> a finite SE on the identified subspace (the well-conditioned near-collinear limit, not minimum-norm; NaN only at rank 0) and warns when a direction is dropped. conley imports _rank_guarded_inv lazily (linalg imports conley one-way, so a top-level import would be circular). Behavior change: a rank-deficient Conley design no longer raises -- it rank-reduces with a warning, uniform with the other structural breads and the covariate convention. Well-conditioned designs are unchanged (the fast path is np.linalg.solve(A, I); R-parity preserved). Excluded after premise verification (documented in TODO.md): HeterogeneousAdoptionDiD's ZtWX is a non-symmetric IV bread (symmetric-PSD _rank_guarded_inv inapplicable); ImputationDiD's vcov is already rank-guarded upstream via solve_ols (its only raw inverse is a Wald F-test statistic with a safe NaN fallback). Tests: per-site near-singular -> finite rank-reduced SE + warning; conley direct-call singular -> finite (not raise) + rank-0 -> NaN + a column-drop == near-collinear-limit anchor; the two TwoStage bread-warning tests re-pointed from the np.linalg.solve seam to the _rank_guarded_inv seam (rank-reduce message). REGISTRY Notes (ContinuousDiD/TwoStageDiD/SpilloverDiD/Conley); CHANGELOG; TODO row retired + had/imputation closed out. No change to estimands, identifying assumptions, point estimates, or the well-conditioned SE path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions · 2026-06-28T23:33:40Z

🔁 AI review rerun (requested by @igerber)

Head SHA: ab2e1b6e92a247362d18de85c67dce0bd8678f2b

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

Affected methods: ContinuousDiD, TwoStageDiD, SpilloverDiD, and Conley spatial-HAC variance.
The rank-guard behavior is documented in REGISTRY.md, including Conley’s raise-to-rank-reduce behavior.
Prior dropped-coordinate inference concern appears resolved: per-coefficient dropped directions now propagate to NaN SE/t-stat/p-value/CI.
TwoStage bootstrap now carries an explicit dropped mask through event-study and group paths.
Only issue found is P3 documentation cleanup: one older TwoStage registry note still says the bread falls back to np.linalg.lstsq.

Methodology

Severity: P3 informational
Impact: Structural bread rank reduction is documented for all affected estimators, so the behavior is not an undocumented methodology deviation.
Locations: docs/methodology/REGISTRY.md:L864, docs/methodology/REGISTRY.md:L1430, docs/methodology/REGISTRY.md:L3675, docs/methodology/REGISTRY.md:L3912
Concrete fix: None required.

Severity: None
Impact: Dropped unidentified coefficients now NaN-propagate instead of reporting zero SE in the changed per-coefficient paths.
Locations: diff_diff/two_stage.py:L3186-L3208, diff_diff/two_stage_bootstrap.py:L562-L584, diff_diff/two_stage_bootstrap.py:L629-L647, diff_diff/spillover.py:L3376-L3403, diff_diff/conley.py:L1119-L1138
Concrete fix: None required.

Code Quality

Severity: None
Impact: No new code-quality blocker found in the changed rank-guard plumbing.
Concrete fix: None required.

Performance

Severity: None
Impact: No material regression found. _rank_guarded_inv() keeps the full-rank fast path as np.linalg.solve(A, I), and these structural breads are small.
Location: diff_diff/linalg.py:L439-L441
Concrete fix: None required.

Maintainability

Severity: None
Impact: The previous maintainability concern is resolved: TwoStage bootstrap uses the explicit dropped mask instead of inferring dropped coefficients from zero-filled bread columns.
Location: diff_diff/two_stage_bootstrap.py:L140-L155, diff_diff/two_stage_bootstrap.py:L562-L584, diff_diff/two_stage_bootstrap.py:L629-L647
Concrete fix: None required.

Tech Debt

Severity: P3 informational
Impact: The excluded HeterogeneousAdoptionDiD and ImputationDiD sites are tracked with rationale in TODO.md, so they are not blockers under the deferred-work rules.
Location: TODO.md:L152-L153
Concrete fix: None required.

Security

Severity: None
Impact: No secrets or security-sensitive changes found in the reviewed diff.
Concrete fix: None required.

Documentation/Tests

Severity: P3
Impact: REGISTRY.md now correctly documents TwoStage’s rank-guarded bread at L1430, but an older nearby note still says the Stage-2 bread falls back to np.linalg.lstsq. That is stale and can confuse future methodology reviews.
Location: docs/methodology/REGISTRY.md:L1488
Concrete fix: Update or remove the stale note so it describes _rank_guarded_inv rank reduction rather than dense lstsq fallback.

Tests were not run: this environment does not have pytest installed (No module named pytest).

igerber force-pushed the fix/structural-rank-guard branch from 0923411 to 013470b Compare June 28, 2026 23:09

igerber force-pushed the fix/structural-rank-guard branch from 013470b to 555e44c Compare June 28, 2026 23:23

igerber force-pushed the fix/structural-rank-guard branch from 555e44c to ab2e1b6 Compare June 28, 2026 23:31

igerber added the ready-for-ci Triggers CI test workflows label Jun 28, 2026

igerber merged commit b5a5452 into main Jun 29, 2026
33 of 34 checks passed

igerber deleted the fix/structural-rank-guard branch June 29, 2026 01:05

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(linalg): rank-guard structural (non-covariate) matrix inverses#576

fix(linalg): rank-guard structural (non-covariate) matrix inverses#576
igerber merged 1 commit into
mainfrom
fix/structural-rank-guard

igerber commented Jun 28, 2026

Uh oh!

github-actions Bot commented Jun 28, 2026

Uh oh!

github-actions Bot commented Jun 28, 2026

Uh oh!

github-actions Bot commented Jun 28, 2026

Uh oh!

github-actions Bot commented Jun 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

igerber commented Jun 28, 2026

Summary

Methodology references (required if estimator / math changes)

Validation

Security / privacy

Uh oh!

github-actions Bot commented Jun 28, 2026

Overall Assessment

Executive Summary

Methodology

Code Quality

Performance

Maintainability

Tech Debt

Security

Documentation/Tests

Path to Approval

Uh oh!

github-actions Bot commented Jun 28, 2026

Overall Assessment

Executive Summary

Methodology

Code Quality

Performance

Maintainability

Tech Debt

Security

Documentation/Tests

Uh oh!

github-actions Bot commented Jun 28, 2026

Overall Assessment

Executive Summary

Methodology

Code Quality

Performance

Maintainability

Tech Debt

Security

Documentation/Tests

Uh oh!

github-actions Bot commented Jun 28, 2026

Overall Assessment

Executive Summary

Methodology

Code Quality

Performance

Maintainability

Tech Debt

Security

Documentation/Tests

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant