Skip to content

docs: Borusyak, Jaravel & Spiess (2024) paper review — ImputationDiD (PR-A)#529

Merged
igerber merged 1 commit into
mainfrom
docs/borusyak-jaravel-spiess-2024-review
Jun 6, 2026
Merged

docs: Borusyak, Jaravel & Spiess (2024) paper review — ImputationDiD (PR-A)#529
igerber merged 1 commit into
mainfrom
docs/borusyak-jaravel-spiess-2024-review

Conversation

@igerber
Copy link
Copy Markdown
Owner

@igerber igerber commented Jun 6, 2026

Summary

  • Add the canonical scholarly review of the BJS (2024) ImputationDiD source paper (docs/methodology/papers/borusyak-jaravel-spiess-2024-review.md) — the Step-1 fidelity artifact (PR-A) of the 2-PR ImputationDiD methodology validation. Version-pinned to the published REStud article; equation/theorem numbering verified against the source PDF (pp. 3266–3274) and the existing REGISTRY ImputationDiD section.
  • Document the Theorem 3 / Eq. 8 auxiliary-aggregator deviation in REGISTRY.md (recognized **Note:** label) and the paper review.
  • Map the review under diff_diff/imputation.py in docs/doc-deps.yaml (mirrors the firpo-possebom docs: Firpo & Possebom (2018) paper review — SCM CI by test inversion (PR-A) #524 precedent).
  • Track PR-B follow-ups in TODO.md (Eq. 8 code reconciliation + test_methodology_imputation.py + didimputation R parity).

Methodology references

  • Method name(s): ImputationDiD
  • Paper / source link(s): Borusyak, Jaravel & Spiess (2024), Revisiting Event-Study Designs: Robust and Efficient Estimation, Review of Economic Studies 91(6), 3253–3285. DOI 10.1093/restud/rdae007.
  • Any intentional deviations from the source (and why): Documented (REGISTRY.md **Note:**). The library's auxiliary tau_tilde_g (Theorem 3 variance) uses a simplified observation-level v_it-weighted mean sum(v·τ̂)/sum(v) instead of the paper's unit-clustered Eq. 8 sum_i[(sum_t v)(sum_t v·τ̂)]/sum_i(sum_t v)^2. The two coincide for the unweighted default paths (overall ATT / single-horizon event-study under cohort_horizon) and diverge for survey-weighted / non-uniform / coarser-partition cases. The simplification stays within Theorem 3's conservative-inference conditions (σ²_τ ≥ 0), so SEs remain valid/conservative — it is simply not the excess-variance-minimizing form. Code reconciliation to the exact Eq. 8 is deferred to PR-B (tracked in TODO.md).

Validation

  • Tests added/updated: No test changes — docs-only PR-A. PR-B adds tests/test_methodology_imputation.py (paper-equation-numbered Verified Components) + a didimputation R parity fixture (tracked in TODO.md).
  • Local AI review: 4 fresh codex rounds, converged ✅ — round 1 surfaced one P1 (the Eq. 8 deviation above, verified against imputation.py:1663-1676), resolved by documenting it; rounds 2–3 resolved minor P3 accuracy items; round 4 had zero actionable findings.
  • tests/test_doc_deps_integrity.py: passing.

Security / privacy

  • Confirm no secrets/PII in this PR: Yes (documentation only; the source PDF is gitignored, not committed).

🤖 Generated with Claude Code

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 6, 2026

✅ Looks good

Executive summary

  • Affected method: ImputationDiD; this PR is docs/TODO/doc-deps only.
  • The Eq. 8 auxiliary-aggregator mismatch is now documented with a **Note:** in REGISTRY.md and tracked in TODO.md, so it is mitigated under the review rules.
  • BJS Theorem 3 / Eq. 8 source framing matches the PR’s main claim: conservative auxiliary residuals are allowed, while Eq. 8 is the unit-clustered weighting target. (academic.oup.com)
  • No P0/P1 findings.
  • pytest was unavailable here, so I could not run tests/test_doc_deps_integrity.py; git diff --check was clean.

Methodology

Finding 1

Severity: P3 informational
Location: docs/methodology/REGISTRY.md:L1338-L1339, TODO.md:L91, diff_diff/imputation.py:L1627-L1664
Impact: The implementation still uses the observation-level sum(v*tau_hat)/sum(v) auxiliary mean, while BJS Eq. 8 uses the unit-clustered form. This is now explicitly documented with a **Note:** and tracked for PR-B, so it is not an approval blocker.
Concrete fix: No required fix in this PR. PR-B should implement the exact unit-clustered Eq. 8, update _compute_auxiliary_residuals_treated’s docstring, and add a diverging-case methodology test.

Finding 2

Severity: P3 documentation precision
Location: docs/methodology/REGISTRY.md:L1339, docs/methodology/papers/borusyak-jaravel-spiess-2024-review.md:L99, TODO.md:L91
Impact: The new wording says coarser cohort/horizon partitions diverge and says a unit gets several observations per group. That is accurate for cohort in some panels, but not generally for horizon, where a unit contributes at most one observation per relative-time group. Divergence for horizon comes from non-uniform v_it, not from multiple observations per unit.
Concrete fix: Narrow the wording to: divergence occurs with non-uniform treated weights, and with coarser partitions when units contribute unequal within-group weight totals or multiple observations, especially aux_partition="cohort".

Code Quality

No findings. The PR does not modify executable code.

Performance

No findings. Documentation-only PR.

Maintainability

Finding

Severity: P3 documentation consistency
Location: docs/methodology/REGISTRY.md:L1354
Impact: The requirements checklist still says [x] Auxiliary model for treated residuals (Equation 8), which conflicts with the new documented deviation just above it. The **Note:** mitigates this, but the checklist can still mislead readers.
Concrete fix: Change the checklist item to [~] and mention the simplified observation-level aggregator with PR-B tracking.

Tech Debt

No blocker. The PR adds appropriate TODO tracking for the Eq. 8 reconciliation and methodology validation at TODO.md:L91 and TODO.md:L181.

Security

No findings. Docs-only changes; no secrets or executable workflow changes observed.

Documentation/Tests

No blocking findings. The new paper review is linked from docs/doc-deps.yaml:L221-L228. I could not run the integrity test because pytest is not installed in this environment.

…(PR-A)

Add the canonical scholarly review of the primary source (DOI 10.1093/restud/rdae007,
Review of Economic Studies 91(6), 3253-3285) as the Step-1 fidelity artifact for the
ImputationDiD methodology validation (PR-A of the 2-PR sequence). Version-pinned to the
published REStud article; equation/theorem numbering verified against the source PDF
(pp. 3266-3274) and the existing REGISTRY.md ImputationDiD section.

Local AI review (4 fresh codex rounds) surfaced and resolved one P1 methodology
finding: the library's _compute_auxiliary_residuals_treated uses a simplified
observation-level v_it-weighted mean for the Theorem 3 auxiliary tau_tilde_g, not the
paper's unit-clustered Equation 8 (verified vs imputation.py:1663-1676 and the PDF
p. 3272). The two coincide for the unweighted default paths (overall ATT / single-
horizon event-study under cohort_horizon) and diverge for survey-weighted / non-uniform
/ coarser-partition cases; the simplification stays conservative (Theorem 3) but is not
the excess-variance-minimizing form.

Per the chosen disposition (document in PR-A, fix code in PR-B):
- docs/methodology/papers/borusyak-jaravel-spiess-2024-review.md: the paper review,
  with the Eq.8 deviation note, qualified checklist, and headline PR-B correction.
- docs/methodology/REGISTRY.md: relabel the aggregator + recognized **Note:** deviation.
- docs/doc-deps.yaml: map the review under diff_diff/imputation.py (mirrors #524).
- TODO.md: track the Eq.8 code reconciliation + the PR-B didimputation parity and
  tests/test_methodology_imputation.py.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@igerber igerber force-pushed the docs/borusyak-jaravel-spiess-2024-review branch from eb50ae8 to c01e2b9 Compare June 6, 2026 15:02
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 6, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: c01e2b93292fe2458ca29af37ac0e849b7070fdb


✅ Looks good

Overall Assessment

No unmitigated P0/P1 findings. This re-review is compatible with approval.

Executive Summary

  • Affected method: ImputationDiD.
  • This PR is documentation/TODO/doc-deps only; no executable estimator, variance, weighting, inference, or default behavior changes.
  • The prior Eq. 8 auxiliary-aggregator mismatch is now explicitly documented with a **Note:** in REGISTRY.md and tracked in TODO.md.
  • The previous wording issue around horizon versus cohort partitions is resolved.
  • docs/doc-deps.yaml now maps the new BJS review to diff_diff/imputation.py.
  • Could not run the doc-deps test because pytest is not installed.

Methodology

Finding 1

Severity: P3 informational
Location: docs/methodology/REGISTRY.md:L1338-L1354, TODO.md:L91, docs/methodology/papers/borusyak-jaravel-spiess-2024-review.md:L90-L100, diff_diff/imputation.py:L1627-L1676
Impact: The implementation still uses the observation-level sum(v_it * tau_hat_it) / sum(v_it) auxiliary mean while the paper frames Theorem 3 conservative inference through an auxiliary model and Eq. 8 grouped averages; the paper also states that the partition can be cohort/period by default when cohorts are large. This deviation is now documented with a registry **Note:** and tracked for PR-B, so it is mitigated under the review rules. (academic.oup.com)
Concrete fix: No required fix in this PR. PR-B should implement the exact unit-clustered Eq. 8 form, update _compute_auxiliary_residuals_treated’s docstring/comments, and add a diverging-case methodology test.

Code Quality

No findings. The PR does not modify executable code.

Performance

No findings. Documentation-only change.

Maintainability

No blocking findings. The previous checklist inconsistency is resolved at docs/methodology/REGISTRY.md:L1354 by changing the auxiliary model row to [~] and pointing to the deviation note.

Tech Debt

No blocker. The deferred Eq. 8 reconciliation is tracked in TODO.md:L91, and the methodology validation/R parity follow-up is tracked in TODO.md:L181.

Security

No findings. No secrets, workflows, dependencies, or executable paths changed.

Documentation/Tests

Finding 1

Severity: P3 informational
Location: docs/doc-deps.yaml:L221-L228, docs/methodology/papers/borusyak-jaravel-spiess-2024-review.md:L123-L131
Impact: The new paper review is correctly mapped under diff_diff/imputation.py; missing methodology tests and R parity are explicitly marked as PR-B deliverables.
Concrete fix: No required fix in this PR.

Validation note: git diff --check passed. python -m pytest tests/test_doc_deps_integrity.py could not run because pytest is not installed in this environment.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jun 6, 2026
@igerber igerber merged commit 75f58de into main Jun 6, 2026
11 of 12 checks passed
@igerber igerber deleted the docs/borusyak-jaravel-spiess-2024-review branch June 6, 2026 15:41
igerber added a commit that referenced this pull request Jun 6, 2026
…-clustered Eq.8 + R parity

Source-validation pass of the Borusyak, Jaravel & Spiess (2024, REStud 91(6))
audit (PR-A #529 added the paper review). Three code corrections in
diff_diff/imputation.py (behavior = SE values change; point estimates unchanged):

1. Untreated v_it weights (Theorem 3 conservative variance). The covariate-free
   path used the BALANCED two-way closed form -(w_i/n0_i + w_t/n0_t - w/N0), wrong
   for the always-unbalanced Omega_0 in staggered designs -> analytical SEs ~27%
   too small. Replaced with the exact projection -A0 (A0'A0)^-1 A1' w (the
   covariate path's method), and fixed that design to keep all unit dummies (the
   prior drop-first-unit/no-intercept design was one rank short -> a further ~1.6%
   bias). SEs now match R didimputation::did_imputation (observed ~1e-10; tests
   assert abs=1e-7). A singular Omega_0 routes to a dense-lstsq fallback (SciPy
   spsolve returns NaN + MatrixRankWarning without raising; promoted to an error
   so the fallback fires under production filters). Bootstrap SEs (which resample
   the same Theorem-3 influence function) may also shift.
2. Auxiliary model (Equation 8): observation-level mean sum(v*tau)/sum(v) -> the
   paper's unit-clustered sum_i(sum_t v)(sum_t v*tau)/sum_i(sum_t v)^2, NaN-safe.
3. Untreated Step-1 residuals preserve NaN for missing FE (symmetric with the
   treated path) instead of a silent fillna(0.0).

Validation:
- tests/test_methodology_imputation.py: paper-equation Verified Components
  (Theorem 1/2; Theorem 3/eqs 6-8 + white-box unit-clustered Eq.8 hand-calc +
  NaN-co-group edge + singular-Omega0 dense-fallback regression; Proposition 5
  K>=H_bar non-ID; Test 1/eq 9 + Proposition 9) and TestImputationDiDParityR
  (overall + per-horizon ATT and SE vs didimputation, no silent skips).
- benchmarks/R/generate_didimputation_golden.R + benchmarks/data/didimputation_*
  (didimputation v0.5.0 goldens).
- tests/test_imputation.py: tightened the coarser-partition conservatism test.
- Full fast suite: 7585 passed (the SE change breaks nothing downstream).

Docs/tracker: REGISTRY ## ImputationDiD (Eq.8 now exact unit-clustered + a
Deviation-from-R note; v_it observation-weights bullet updated to the exact
projection); paper review flipped to "implemented"; METHODOLOGY_REVIEW.md row ->
Complete (Verified Components / Corrections Made / Deviations / R Comparison
Results); CHANGELOG entry; TODO PR-B rows removed + follow-ups tracked (LOO
refinement, projection-factorization caching, covariate-path R parity).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant