Skip to content

[Mirror] LWDiD estimator - CI-review mirror of PR #588 (do not merge)#690

Open
igerber wants to merge 1 commit into
mainfrom
mirror/pr588-lwdid
Open

[Mirror] LWDiD estimator - CI-review mirror of PR #588 (do not merge)#690
igerber wants to merge 1 commit into
mainfrom
mirror/pr588-lwdid

Conversation

@igerber

@igerber igerber commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

Methodology references (required if estimator / math changes)

  • Method name(s): LWDiD (rolling-transformation DiD)
  • Paper / source link(s): Lee & Wooldridge, SSRN 4516518 (rev. June 8, 2026) and SSRN 5325686 (Feb 3, 2026); docs/methodology/REGISTRY.md ## LWDiD section; canonical reviews in docs/methodology/papers/lee-wooldridge-{2025,2026}-review.md
  • Any intentional deviations from the source (and why): tracked as xfail acceptance criteria in tests/test_methodology_lwdid.py and step-2 items in feat: add LWDiD estimator (Lee & Wooldridge 2025, 2026) #588 (IPW IF centering, composite-outcome aggregation, Appendix D event study, RI convention)

Validation

  • Tests added/updated: contributor's 9 tests/test_lwdid*.py files (179 pass / 43 importorskip-skipped); tests/test_methodology_lwdid.py active at 30 pass / 18 xfail; slow MC bias-ordering test passes
  • Backtest / simulation / notebook evidence (if applicable): Prop 99 Table 3 / Table 4 / Table A1 replicate to printed precision on this head

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

Maintainer rebase onto current main (igerber, 2026-07-17), per plan agreed
in PR #588: dropped committed datasets (tutorial now uses the checksummed
load_prop99()/load_walmart() loaders on main), kept the maintainer-authored
paper reviews and references entries from #685, removed the lwdid dev
dependency (external reference implementations stay environmental,
importorskip-gated), renumbered tutorial 26 -> 27.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Overall Assessment

⚠️ Needs changes — no P0s found, but multiple unmitigated P1 methodology/inference issues remain in the new LWDiD estimator.

Executive Summary

  • Affected methods: LWDiD common/staggered RA/IPW/IPWRA paths, staggered aggregation, exact inference, and wild-cluster bootstrap.
  • The main blockers are undocumented mismatches against docs/methodology/REGISTRY.md and the Lee-Wooldridge 2025/2026 contracts.
  • Existing xfail markers document awareness, but they are not REGISTRY Note/Deviation labels or TODO.md deferrals, and P1 methodology/SE defects are not deferrable under the review rules.
  • I did not find a NaN-inference inconsistency in the main safe_inference() paths.
  • No security issue or accidental secret found in the changed files.

Methodology

  1. Severity: P1 — Staggered overall SE uses an invalid independence-across-cohorts aggregation.
    Location: diff_diff/lwdid.py:L927-L933, diff_diff/lwdid.py:L995-L1042, diff_diff/lwdid_results.py:L357-L396
    Impact: LW 2026 requires the overall staggered tau_omega SE from the composite-outcome regression in equations 7.18/7.19, which accounts for correlation across cohort effects. The implementation instead uses sqrt(sum(w_g^2 * se_g^2)), assuming independent cohort estimates, producing incorrect SE/p-value/CI.
    Concrete fix: Build the composite outcome for treated and never-treated/control units, run the single cross-sectional regression, and derive ATT/SE/inference from that regression.

  2. Severity: P1 — IPW influence-function SE is translation-variant.
    Location: diff_diff/lwdid.py:L2042-L2088, tests/test_methodology_lwdid.py:L121-L126, tests/test_methodology_lwdid.py:L485-L488
    Impact: A constant shift to all post outcomes should not change the SE, but the IPW IF uses uncentered outcome terms (y rather than centered treated/control residual components). This silently changes reported IPW SEs under an irrelevant outcome translation.
    Concrete fix: Use the centered Hájek ATT IF, include the propensity-score correction with centered control residual terms, center the final IF before variance, and make the translation-invariance xfail pass.

  3. Severity: P1 — Required treatment/cohort assumption checks are missing.
    Location: diff_diff/lwdid.py:L262-L281, diff_diff/lwdid.py:L360-L428, diff_diff/lwdid.py:L736-L755
    Impact: The registry requires absorbing treatment and mutually exclusive/time-invariant staggered cohorts. fit() validates only binary treatment, then the staggered path ignores the supplied treatment column. Non-absorbing treatment, time-varying cohorts, or cohort/treatment inconsistencies can silently produce invalid estimates.
    Concrete fix: In fit(), validate monotone absorbing treatment by unit, common timing when cohort is None, cohort time-invariance, and consistency between cohort and observed treatment onset when cohort is supplied.

  4. Severity: P1 — never_treated staggered controls allow a singleton NT pool.
    Location: diff_diff/lwdid.py:L746-L754, tests/test_methodology_lwdid.py:L625-L644
    Impact: LW 2026 requires N_infinity >= 2 for NT-only controls. The code only rejects zero never-treated units, so exact NT-only inference can be reported for an invalid singleton control pool.
    Concrete fix: Require at least two unique never-treated units when control_group="never_treated".

  5. Severity: P1 — Staggered RA per-cohort degrees of freedom are too large.
    Location: diff_diff/lwdid.py:L897-L902
    Impact: _estimate_ra() returns K_controls, and the common-timing path correctly uses N - K - 2; the staggered path uses N - K, making per-cohort p-values and CIs too liberal.
    Concrete fix: Mirror the common-timing df logic for each cohort; for clustered inference use G - 1.

  6. Severity: P1 — Wild-cluster bootstrap with controls imposes the null using intercept-only residuals.
    Location: diff_diff/lwdid_wild_bootstrap.py:L677-L728
    Impact: When controls are supplied, the restricted bootstrap model should retain controls and drop only treatment. The current intercept-only restricted model creates the wrong bootstrap distribution and p-values whenever controls explain outcomes.
    Concrete fix: Build X_restricted = [1, controls] when impose_null=True, fit restricted residuals from that model, and add a test where controls materially affect WCB p-values.

Code Quality

  • Severity: P2 — Unknown kwargs are silently ignored in the functional wrapper.
    Location: diff_diff/lwdid.py:L3167-L3176
    Impact: Typos in public API calls can be silently dropped.
    Concrete fix: Explicitly consume known compatibility aliases and raise ValueError for remaining unknown kwargs.

Performance

  • Severity: P3 — Bootstrap repeatedly scans the dataframe for unit indices.
    Location: diff_diff/lwdid.py:L2670-L2676, diff_diff/lwdid.py:L2764-L2772
    Impact: Unit bootstrap is unnecessarily slow on large panels.
    Concrete fix: Precompute unit -> row_indices once before the bootstrap loop.

Maintainability

  • Severity: P3 — New core estimator file is already over 3k lines.
    Location: diff_diff/lwdid.py
    Impact: Validation, transformations, estimators, bootstrap, and wrappers are tightly coupled in one large file.
    Concrete fix: Split into focused modules after the methodology fixes land: validation, transformations, estimators/inference, bootstrap, and wrapper API.

Tech Debt

  • Severity: P1 — Existing xfails track P1 methodology gaps but do not mitigate them.
    Location: tests/test_methodology_lwdid.py:L121-L140, tests/test_methodology_lwdid.py:L438-L445, tests/test_methodology_lwdid.py:L625-L644
    Impact: The xfails correctly identify unresolved methodology defects, but they are not acceptable deferrals for incorrect SEs or missing assumption checks.
    Concrete fix: Convert the P1 xfails above into passing tests with the implementation fixes.

Security

  • Severity: P3 — No security findings.
    Impact: No secrets or unsafe file/network behavior found in the reviewed diff.
    Concrete fix: No action required.

Documentation/Tests

  • Severity: P2 — Docs claim absorbing-treatment validation that fit() does not enforce.
    Location: docs/api/lwdid.rst:L419-L421, diff_diff/lwdid.py:L262-L281
    Impact: Users are told non-absorbing treatment raises ValueError, but the implementation accepts it silently.
    Concrete fix: Prefer fixing the code; otherwise downgrade the docs only if the limitation is explicitly documented in REGISTRY/TODO.

  • Severity: P3 — Tutorial numbering mismatch.
    Location: docs/tutorials/27_lwdid.ipynb:L8
    Impact: Notebook says “Tutorial 26” while the file/index are tutorial 27.
    Concrete fix: Rename the notebook heading to Tutorial 27.

Path To Approval

  1. Add fit-time validation for absorbing treatment, common timing, time-invariant cohorts, cohort/treatment consistency, and N_infinity >= 2 for NT-only controls.
  2. Replace staggered overall aggregation with the LW 2026 composite-outcome regression and use its SE/inference.
  3. Correct staggered RA degrees of freedom and clustered df handling.
  4. Fix the IPW influence function so IPW SE is translation-invariant; remove the xfail.
  5. Fix wild-cluster bootstrap restricted residuals to retain controls under the null.
  6. Update the affected methodology tests from xfail to passing and correct the tutorial/docs mismatches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants