Overall Assessment: ⚠️ Needs changes

Executive Summary

• P1: The new CS covariate reg path can now raise on underdetermined control cells (n_control < p + 1) even under rank_deficient_action="warn" / "silent".
• Methodology direction is otherwise sound: routing OR nuisance fits through scale-equilibrated solve_ols is documented as an implementation-choice fix, not a source-material mismatch.
• Registry has a stale top-level note still saying CS/SDDD OR solves are “not yet equilibrated,” contradicting the new method-specific notes.
• Existing tests still encode stale comments/coverage around CS reg mixed-scale duplicate behavior.

Methodology

Finding 1

Severity: P1
Location: diff_diff/staggered.py:L1402-L1407, diff_diff/linalg.py:L824-L829
Impact: The optimized CallawaySantAnna(estimation_method="reg") covariate path now calls solve_ols() directly. solve_ols() raises ValueError when n < k before rank detection/drop handling, so sparse control cells with more covariate columns than controls can crash even when the estimator is configured to warn/silently drop rank-deficient columns. This conflicts with the registry’s rank-deficiency contract for non-error modes and is a new edge-case regression from the removed reduced-lstsq local path.
Concrete fix: Before calling solve_ols() in _compute_all_att_gt_covariate_reg, handle pair_X_ctrl.shape[0] < pair_X_ctrl.shape[1] by detecting kept columns and solving the reduced design, then expanding dropped coefficients back to NaN/zero-for-prediction. Alternatively, update solve_ols() to perform rank dropping before the n < k guard for warn/silent. Add a regression test with, for example, 2 never-treated controls and 3 covariates under CS reg.

Code Quality

No findings.

Performance

No findings. The Cholesky-cache removal is a documented numerical robustness tradeoff and the changed paths use return_vcov=False.

Maintainability

No findings beyond the documentation drift below.

Tech Debt

No blocker findings. The removed TODO entry is appropriate if the P1 edge case above is fixed.

Security

No findings.

Documentation/Tests

Finding 2

Severity: P2
Location: docs/methodology/REGISTRY.md:L81
Impact: The registry still says CallawaySantAnna and StaggeredTripleDifference OR nuisance fits are “not yet equilibrated,” while later notes say they now route through solve_ols. Since the registry is the review source of truth, this contradiction will mislead future methodology reviews.
Concrete fix: Update the top-level scale-invariance note to include CS/SDDD in the now-covered solve_ols scope, or point readers to the updated CS/SDDD notes.

Finding 3

Severity: P3
Location: tests/test_staggered.py:L1481-L1514, tests/test_staggered.py:L1560-L1562
Impact: Existing comments still describe CS reg as using an un-equilibrated local OR solve and skip the mixed-scale order-invariance assertion for reg. That is stale after this PR and weakens coverage of the fixed behavior.
Concrete fix: Update the comments and extend the assertion to reg if the new behavior is intended to guarantee order-invariant mixed-scale duplicate handling.

Path to Approval

1. Fix the CS optimized covariate reg path so rank_deficient_action="warn" / "silent" does not raise when a control-cell design has fewer rows than columns.
2. Add a targeted test for that underdetermined control-cell case.
3. Update the stale registry scale-invariance note at docs/methodology/REGISTRY.md:L81.

🔁 AI review rerun (requested by @igerber)

Head SHA: 35dca444b5a9e6ff4a6b71bf160c40f92f4f83c9

Overall Assessment: ⚠️ Needs changes

Executive Summary

• Prior P1 is only partially resolved: CS reg underdetermined control cells no longer crash, but the new branch bypasses the documented R-style dropped-column solve.
• The main full-rank scale-equilibration change is methodologically sound: replacing normal-equation Cholesky with scale-equilibrated SVD/solve_ols is an implementation choice aligned with the registry.
• Registry/TODO drift from the previous review is fixed.
• No new inline inference, parameter propagation, security, or performance issues found.
• One stale test still patches scipy.linalg.lstsq, which no longer exercises the optimized OR solver path.

Methodology

Finding 1

Severity: P1
Location: diff_diff/staggered.py:L1402-L1424, docs/methodology/REGISTRY.md:L538-L542
Impact: The underdetermined CS covariate reg fallback calls _equilibrated_lstsq(pair_X_ctrl, control_change) on the full n < k design. That avoids the solve_ols() crash, but it uses a minimum-norm solution with finite coefficients for all columns instead of the registry’s R-style contract: detect rank deficiency, drop unidentified columns, and set dropped coefficients to NaN before zeroing for prediction. In underdetermined OR cells, extrapolation to treated covariates is not unique, so this can silently change ATT(g,t) and the SE that follows it relative to the documented lm()/solve_ols column-drop convention. The nearby warning says dropped columns are excluded, but this branch does not exclude them.
Concrete fix: In the n < k branch, run _detect_rank_deficiency(pair_X_ctrl), build kept_cols, solve _equilibrated_lstsq(pair_X_ctrl[:, kept_cols], control_change), then expand dropped coefficients to NaN and let the existing NaN zero-fill handle prediction. Alternatively, move this rank-drop-before-n < k behavior into solve_ols(). Add a regression test where the minimum-norm full-design prediction differs from the rank-reduced prediction, and assert the CS ATT matches the rank-reduced/R-style path.

Code Quality

No findings.

Performance

No findings.

Maintainability

No findings.

Tech Debt

No findings. The removed TODO row is appropriate once the P1 underdetermined-cell contract is fixed.

Security

No findings.

Documentation/Tests

Finding 2

Severity: P3
Location: tests/test_staggered.py:L1009-L1034
Impact: test_nan_cell_preserved_not_dropped still patches scipy.linalg.lstsq, but the optimized OR path now reaches solve_ols / the imported scipy_lstsq alias, so this mock may not poison any OR solve. The test can pass without exercising the NaN-cell guard it describes.
Concrete fix: Patch diff_diff.staggered.solve_ols as the later tests do, or patch the actual diff_diff.linalg solver seam being exercised, and assert call_count > 0 plus the expected NaN cell.

Path to Approval

1. Replace the underdetermined CS reg fallback with a rank-reduced, dropped-column solve before prediction.
2. Add a targeted underdetermined-cell test that checks R-style dropped-column semantics, not only “no crash.”
3. Update the stale test_nan_cell_preserved_not_dropped mock to patch the active solver seam.

🔁 AI review rerun (requested by @igerber)

Head SHA: e252f16330c6539826709ebe9bd0a9ad67e1c3cb

Overall Assessment: ✅ Looks good

Executive Summary

• Affected methods: CallawaySantAnna covariate reg/dr OR nuisance fits and StaggeredTripleDifference._compute_or.
• Prior P1 is resolved: the underdetermined CS reg path now rank-drops columns before solving, instead of using a full minimum-norm n < k solve.
• The Cholesky-to-solve_ols change is documented in the Methodology Registry and is a valid numerical implementation change, not a methodology defect.
• Prior stale-test P3 is resolved: tests now patch the active diff_diff.staggered.solve_ols seam and assert the mock fires.
• I could not run tests locally because pytest is not installed in this review environment.

Methodology

Finding 1

Severity: P3 informational
Location: diff_diff/staggered.py:L1393-L1429, diff_diff/staggered.py:L2806-L2822, diff_diff/staggered_triple_diff.py:L1601-L1616, docs/methodology/REGISTRY.md:L538-L542, docs/methodology/REGISTRY.md:L2382-L2383
Impact: The OR nuisance fits now route through the documented scale-equilibrated solve_ols/SVD path. This matches the registry’s stated scope and is an implementation choice aligned with the documented R lm()/QR family.
Concrete fix: None required.

Finding 2

Severity: P3 informational
Location: diff_diff/staggered.py:L1402-L1422, tests/test_staggered.py:L1223-L1280
Impact: The previous P1 is addressed. The n_control < n_covariates + 1 CS reg path now detects rank deficiency, keeps the identified pivot columns, solves the reduced design with _equilibrated_lstsq, and leaves dropped coefficients as NaN before prediction zero-fill.
Concrete fix: None required.

Code Quality

No findings. The removed cho_cache plumbing is consistent with the shared solver routing, and no new inline inference anti-pattern was introduced.

Performance

No findings. The PR removes a Cholesky cache in favor of repeated scale-equilibrated solves, but these are small nuisance regressions and there is no clear performance defect in the changed code.

Maintainability

No findings. Routing estimator-local OR solves through the shared linear algebra backend reduces duplicated numerical behavior.

Tech Debt

No findings. Removing the TODO row for CS/SDDD OR scale-equilibration is appropriate now that the implementation and registry notes have been updated.

Security

No findings. No secrets or security-sensitive changes observed in the diff.

Documentation/Tests

Finding 1

Severity: P3 informational
Location: tests/test_staggered.py:L1002-L1066, tests/test_staggered.py:L1223-L1280, tests/test_methodology_callaway.py:L1925-L1988, tests/test_methodology_staggered_triple_diff.py:L822-L860
Impact: The previous stale mock is fixed, and the added tests cover the scale-invariance and underdetermined-cell contracts. I could not independently execute them because pytest is unavailable in this environment (No module named pytest).
Concrete fix: None for the PR; rely on CI to run the affected tests.