Add EfficientDiD estimator (Chen, Sant'Anna & Xie 2025)

[igerber]

Summary

• Implement the semiparametrically efficient ATT estimator from Chen, Sant'Anna & Xie (2025), Phase 1 (no-covariates path)
• Achieves the efficiency bound by optimally weighting across pre-treatment periods and comparison groups via the inverse of Omega*
• Supports both PT-All (overidentified, efficiency gains) and PT-Post (just-identified, reduces to CS)
• Includes analytical SEs from EIF, multiplier bootstrap inference, and event study / group aggregations
• 42 tests across 4 tiers: core correctness, weight behavior, bootstrap, and simulation validation

New files

File	Purpose	Lines
diff_diff/efficient_did.py	Main EfficientDiD class	~675
diff_diff/efficient_did_weights.py	Omega* matrix, efficient weights, EIF	~537
diff_diff/efficient_did_results.py	EfficientDiDResults dataclass	~272
diff_diff/efficient_did_bootstrap.py	Multiplier bootstrap mixin	~284
tests/test_efficient_did.py	Test suite (42 tests)	~790

Methodology references (required if estimator / math changes)

• Method name(s): Efficient Difference-in-Differences (EDiD)
• Paper / source link(s): Chen, X., Sant'Anna, P. H. C., & Xie, H. (2025). "Efficient Difference-in-Differences and Event Study Estimators"
• Paper review: docs/methodology/papers/chen-santanna-xie-2025-review.md
• Key equations: Omega* (Eq 3.12), efficient weights (Eq 3.13/4.3), generated outcomes (Eq 3.9/4.4), EIF (Thm 3.2), event study aggregation (Eq 4.5)
• Any intentional deviations from the source: Uses multiplier bootstrap on EIF values rather than nonparametric clustered bootstrap (asymptotically equivalent, computationally cheaper, consistent with CS pattern)

Validation

• Tests added: tests/test_efficient_did.py — 42 tests in 4 tiers
  • Tier 1 (core): basic fit, zero/positive effects, PT-Post ≈ CS equivalence, aggregation, validation, sklearn compat, output formats, NaN inference, pre-treatment placebo
  • Tier 2 (weights/edges): weight behavior under iid/AR errors, valid triple enumeration, all-units-treated, anticipation
  • Tier 3 (bootstrap): finite SEs, aggregation, coverage
  • Tier 4 (simulation): unbiasedness, efficiency gain vs CS with rho=-0.5, weight adaptation to error structure, analytical-bootstrap SE consistency
• Full test suite: 1531 passed, 94 skipped, 0 failures (excluding slow TROP tests)
• Linting: ruff check clean, mypy clean, black formatted

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

Overall Assessment
⛔ Blocker

Executive Summary

• P0: PT-All comparisons can include already-treated cohorts, violating the “not‑yet‑treated” requirement and biasing ATT(g,t).
• P1: PT-Post path uses a universal baseline Y_1, so it does not reduce to the documented single‑baseline DiD at g-1.
• P1: Aggregated SEs (overall/event‑study) omit the WIF correction from the registry EIF, likely understating uncertainty.
• P1: All‑treated panels are not handled per the registry (last cohort as control), leading to NaNs/invalid estimates.
• P1: anticipation is ignored in event‑study relative time and bootstrap aggregation, creating inconsistent parameter behavior.

Methodology

• P0 diff_diff/efficient_did_weights.py:L68-L95 PT‑All enumeration never checks that comparison cohorts are untreated at target time t (no target_t < g' - anticipation filter), even though the registry requires “not‑yet‑treated cohorts.” Impact: already‑treated cohorts can enter the comparison set, biasing ATT(g,t) and violating PT‑All identification. Fix: filter candidate groups to those with effective_gp > target_t (or gp > target_t + anticipation) and add a regression test for a case with g' <= t. docs/methodology/REGISTRY.md:L475-L477
• P1 diff_diff/efficient_did_weights.py:L343-L413 The PT‑Post path uses Y_t - Y_1 in generated outcomes regardless of t_pre = g-1, so it does not reduce to the single‑baseline DiD described in the registry. Impact: under PT‑Post (which does not guarantee pre‑treatment trends), using the universal baseline can be biased. Fix: special‑case PT‑Post to construct outcomes/covariances with baseline t_pre (replace Y_1 with Y_{t_pre}) or delegate to Callaway‑Sant’Anna for PT‑Post. docs/methodology/REGISTRY.md:L475-L477
• P1 diff_diff/efficient_did.py:L518-L610 Aggregated SEs for overall ATT and ES(e) only average EIFs; the registry’s EIF for ES(e) includes additional weight‑uncertainty terms (WIF correction). Impact: aggregated SEs are likely understated, leading to over‑confident inference. Fix: implement the EIF^{es(e)} correction (and analogous overall correction) per registry and propagate to bootstrap. docs/methodology/REGISTRY.md:L573-L578
• P1 diff_diff/efficient_did.py:L236-L279 and diff_diff/efficient_did_weights.py:L395-L413 When there are no never‑treated units, the code only warns but still computes means/covariances on an empty never‑treated group, yielding NaNs. Impact: all‑treated panels are unsupported despite registry guidance. Fix: implement the registry fallback (drop last period, treat last cohort as control) or hard‑fail with guidance; also exclude gp=inf pairs when n_control_units==0. docs/methodology/REGISTRY.md:L585-L586
• P1 diff_diff/efficient_did.py:L94-L98 and diff_diff/efficient_did_bootstrap.py:L1-L205 The registry marks cluster bootstrap SEs as supported, but cluster raises NotImplementedError and only a multiplier bootstrap is provided. Impact: documented inference path is missing; small‑sample guidance in the registry cannot be followed. Fix: implement cluster bootstrap/cluster‑robust SEs or update the registry to reflect multiplier‑only inference. docs/methodology/REGISTRY.md:L558-L565

Code Quality

• P1 diff_diff/efficient_did.py:L561-L621 and diff_diff/efficient_did_bootstrap.py:L219-L258 anticipation shifts post‑treatment filtering (t >= g - anticipation) but event‑study relative time is still e = t - g (no anticipation shift). Impact: event‑study labels and aggregation are inconsistent with the effective treatment boundary, violating parameter‑interaction expectations. Fix: define e = t - (g - anticipation) and apply the same shift in bootstrap aggregation; add tests for this interaction.
• P1 diff_diff/efficient_did_results.py:L71-L93 The results object omits key estimator parameters (anticipation, n_bootstrap, bootstrap_weights, seed, cluster), unlike other result classes. Impact: results are not self‑describing and violate the “new param” propagation rule. Fix: add these fields to EfficientDiDResults, populate them in fit, and surface in summary().

Performance
No issues found.

Maintainability
No issues found.

Tech Debt
No issues found.

Security
No issues found.

Documentation/Tests

• P2 tests/test_efficient_did.py does not cover the critical edge cases above (not‑yet‑treated exclusion at target time, PT‑Post baseline correctness, anticipation‑shifted event‑study time, and all‑treated fallback). Impact: regressions in identification/inference can slip through. Fix: add targeted tests for each of these behaviors and assert non‑NaN outputs where the registry expects a defined estimator.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 6184ed3b8a805c792b6d7b19a34f466b1ea9e408

---

Overall Assessment

• ⛔ Blocker

Executive Summary

• The PT-All moment set is implemented incorrectly: enumerate_valid_triples() drops the same-cohort moments and restricts t_pre to be pre-treatment for the target cohort, so this is not yet the Chen–Sant’Anna–Xie efficient estimator under PT-All. diff_diff/efficient_did_weights.py:18, diff_diff/efficient_did.py:305, docs/methodology/REGISTRY.md:597. citeturn6view0
• That bug is not cosmetic: it makes identifiable PT-All cases degenerate to NaN (for example, the registry’s g=2 single-pre-period case) and drops valid overidentifying moments that are supposed to deliver the efficiency gains. diff_diff/efficient_did.py:316, docs/methodology/REGISTRY.md:597. citeturn6view0
• Bootstrap event-study inference fixes cohort-share weights instead of bootstrapping the full aggregated EIF, so the event-study bootstrap path misses the paper’s weight-uncertainty terms; the same shortcut is reused for the overall summary. diff_diff/efficient_did_bootstrap.py:124, diff_diff/efficient_did_bootstrap.py:219, diff_diff/efficient_did.py:708, docs/methodology/REGISTRY.md:589. citeturn3view2
• Edge-case handling is inconsistent: PT-Post + anticipation can silently drop cohorts, and bootstrap aggregation does not mirror the analytical finite-cell filtering, so mixed-validity panels can change or invalidate summaries without a clear signal. diff_diff/efficient_did.py:291, diff_diff/efficient_did.py:602, diff_diff/efficient_did_bootstrap.py:104.
• The tests currently lock in the wrong PT-All interpretation (g=2 treated as unidentified; only common pre-periods treated as valid), so CI will pass the core methodology regression instead of catching it. tests/test_efficient_did.py:519, tests/test_efficient_did.py:532, tests/test_efficient_did.py:565. citeturn6view0

Methodology

• Severity P0 — enumerate_valid_triples() implements the wrong PT-All index set. It excludes g' = g entirely (diff_diff/efficient_did_weights.py:77) and then forces every t_pre to satisfy t_pre < g (diff_diff/efficient_did_weights.py:89), which removes the same-group moments and the paper’s additional bridging moments for g' != g. Impact: PT-All is no longer the paper’s overidentified estimator, efficiency claims are unsupported, and identifiable designs can become spuriously unidentified. Concrete fix: define the PT-All score set exactly as in the paper—include same-group terms (g, t_pre) across the target group’s available pre-periods, and for g' != g allow t_pre up to g' - 1 without the t_pre < g restriction—then recompute Omega*, generated outcomes, and EIFs on that full set. diff_diff/efficient_did.py:335, diff_diff/efficient_did_weights.py:116, diff_diff/efficient_did_weights.py:423. citeturn6view0
• Severity P1 — Bootstrap event-study inference omits the aggregation-weight part of the EIF. Analytical event-study SEs add a WIF correction in diff_diff/efficient_did.py:708, but _prepare_es_agg_boot() and _run_multiplier_bootstrap() only resample fixed-weight averages of perturbed ATT cells in diff_diff/efficient_did_bootstrap.py:124 and diff_diff/efficient_did_bootstrap.py:247. Impact: bootstrap SEs/CIs/p-values for event-study summaries are too optimistic relative to the paper’s EIF and inconsistent with the analytical path; the same issue carries into overall_att in diff_diff/efficient_did_bootstrap.py:108. Concrete fix: bootstrap a combined aggregated IF, following the repo’s existing pattern in diff_diff/staggered_aggregation.py:165 and diff_diff/staggered_bootstrap.py:518, rather than only re-averaging cell-level perturbations. citeturn3view2

Code Quality

• Severity P1 — Empty/invalid-cell handling is inconsistent across paths. Under PT-Post, fit() silently skips a cohort when g - 1 - anticipation is outside the sample (diff_diff/efficient_did.py:291), but analytical aggregation later filters non-finite cells (diff_diff/efficient_did.py:602, diff_diff/efficient_did.py:671, diff_diff/efficient_did.py:760) while bootstrap aggregation does not (diff_diff/efficient_did_bootstrap.py:104, diff_diff/efficient_did_bootstrap.py:227, diff_diff/efficient_did_bootstrap.py:268). Impact: a cohort can disappear from the estimand without warning, and one NaN ATT cell can poison bootstrap overall / event-study / group inference even when the analytical aggregation stays finite. Concrete fix: surface unidentified cells explicitly as warned NaNs instead of silently continue-ing, and reuse the same finite-cell keeper logic in both analytical and bootstrap aggregation code.

Performance

• No material performance findings in the changed code beyond the methodological blockers above.

Maintainability

• No separate maintainability finding beyond the core PT-All abstraction issue; once the moment-set logic is corrected, the current file split is reasonable.

Tech Debt

• No separate tech-debt finding beyond the methodology and edge-case issues above.

Security

• No security findings in the changed files.

Documentation/Tests

• Severity P1 — The new tests and helper docstring encode the same incorrect PT-All interpretation. tests/test_efficient_did.py:519 only expects (inf, 2) for a single treated cohort, tests/test_efficient_did.py:532 reduces the staggered PT-All case to common pre-periods only, and tests/test_efficient_did.py:565 explicitly asserts that g=2 has no valid PT-All moments. The docstring in diff_diff/efficient_did_weights.py:28 says the same thing. Impact: the regression suite will keep approving the core methodology bug, and a future correct implementation will look like a regression. Concrete fix: rewrite the valid-triples tests from the paper’s staggered IF index set, add a regression that PT-All with a single pre-period returns a finite ATT with weight 1, and add a bootstrap edge-case test for mixed finite/NaN ATT cells. citeturn6view0

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 322377fa4659789061bfeeeca5fc1929e85ed64e

---

Overall Assessment: ⚠️ Needs changes

Executive Summary

• Affected methods: EfficientDiD ATT(g,t), the new overall/event-study aggregation layer, and bootstrap inference.
• Aggregate bootstrap inference is not aligned with the analytical EIF path: the bootstrap omits the weight-influence-function correction for overall and event-study outputs.
• The default overall_att is not the Chen, Sant’Anna & Xie overall parameter in the Methodology Registry; it is a library-specific simple aggregation that overweights early cohorts.
• n_bootstrap exposes multiplier EIF bootstrap as the estimator’s general bootstrap inference, but the registry still documents clustered resampling as the paper bootstrap.
• set_params() and the new WIF regression test are too weak to catch assumption/inference misconfiguration.

Methodology

• Severity P1 | Impact: Aggregate bootstrap inference drops the WIF/combined-IF term. The new bootstrap path re-aggregates perturbed ATT(g,t) values with fixed cohort weights in diff_diff/efficient_did_bootstrap.py:L119-L145 and diff_diff/efficient_did_bootstrap.py:L230-L273, while the analytical path explicitly adds WIF for overall and event-study SEs in diff_diff/efficient_did.py:L537-L725. The local Callaway-Sant’Anna precedent bootstraps the combined IF, not fixed-weight aggregates, in diff_diff/staggered_bootstrap.py:L291-L320 and diff_diff/staggered_bootstrap.py:L518-L528. As written, n_bootstrap>0 can understate or otherwise misstate overall/event-study SEs, CIs, and p-values. | Concrete fix: build and perturb a combined IF for each aggregate, exactly as the CS bootstrap does, instead of bootstrapping only cell-level ATT perturbations.
• Severity P1 | Impact: The default overall_att estimand does not match the source material. The registry defines the overall summary as ES_avg = (1/N_E) * sum_e ES(e) in docs/methodology/REGISTRY.md:L549-L560, but _aggregate_overall() uses a simple average over post-treatment (g,t) cells with pi_g repeated once per cell in diff_diff/efficient_did.py:L585-L641, and the results object documents that as the main “overall ATT” in diff_diff/efficient_did_results.py:L33-L36. That gives earlier cohorts extra weight solely because they have more post periods. | Concrete fix: either implement Eq. 2.3 for the default overall summary, or rename/document this as a library-specific simple aggregation rather than the paper’s overall parameter.
• Severity P1 | Impact: The public bootstrap API is an undocumented deviation from the methodology registry. The estimator advertises multiplier bootstrap inference through n_bootstrap/bootstrap_weights in diff_diff/efficient_did.py:L58-L66 and diff_diff/efficient_did.py:L443-L454, and implements it in diff_diff/efficient_did_bootstrap.py:L1-L80. But the registry still says the paper’s bootstrap is nonparametric clustered bootstrap, with multiplier bootstrap reserved for simultaneous bands, in docs/methodology/REGISTRY.md:L574-L582. For a new estimator/SE path, that is a methodology deviation that is not documented in the committed docs. | Concrete fix: either implement the paper bootstrap, or explicitly record the multiplier bootstrap as a deliberate deviation in REGISTRY.md and the estimator docstrings, including when it is intended to substitute for clustered resampling.

Code Quality

• Severity P2 | Impact: set_params() does not revalidate constrained parameters in diff_diff/efficient_did.py:L123-L130. Because fit() treats anything other than exact "post" as the PT-All branch in diff_diff/efficient_did.py:L291-L303, a typo like set_params(pt_assumption="POST") silently changes the identification assumption instead of failing fast. Existing estimators revalidate after set_params() in diff_diff/continuous_did.py:L140-L146. | Concrete fix: factor the constructor checks into a private validator and call it from both __init__() and set_params().

Performance

• No material findings in the diff.

Maintainability

• No material findings beyond the methodology/documentation drift above.

Tech Debt

• No additional material findings beyond the undocumented bootstrap/aggregation deviations above.

Security

• No material findings in the diff.

Documentation/Tests

• Severity P2 | Impact: The new WIF regression test does not actually compare a WIF path to a no-WIF path. Both branches call the same fit() implementation in tests/test_efficient_did.py:L851-L867, and overall_se always comes from _aggregate_overall() with WIF in diff_diff/efficient_did.py:L413-L417 and diff_diff/efficient_did.py:L585-L641. That test will pass even if the specific WIF regression it is meant to protect is broken. | Concrete fix: compare against a genuinely naive no-WIF aggregation helper or a hand-computed benchmark, and add a bootstrap regression test that asserts the aggregate path uses the combined IF.
• Review note: I did not run the Python test suite in this environment because the available interpreter cannot import numpy.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 3102a7c03d5c0fdb0ac15ae485f16a7902bcf498

---

Overall assessment: ⚠️ Needs changes

Executive Summary

• Affected methods: EfficientDiD no-covariates ATT(g,t), overall ATT aggregation, event-study aggregation, and multiplier bootstrap inference.
• Cell-level inference is wired through safe_inference(). I did not find the new inline t_stat = effect / se anti-pattern in the changed files.
• The main methodology issue is aggregated bootstrap inference: overall and event-study bootstrap paths drop the same weight-influence terms that the analytical path and the new Registry formula rely on.
• balance_e handling is inconsistent between analytical and bootstrap event-study aggregation when the anchor horizon is NaN, so the reported point estimate and its bootstrap interval can target different cohorts.
• Input validation is too loose for a method that requires a short balanced panel: duplicated (unit, time) rows are silently collapsed by pivot_table(..., aggfunc="first").
• Public docs and coverage are incomplete for a new exported estimator. There is no balance_e coverage, and the PR-cited methodology review file is not present in the repo.

Methodology
Affected methods: EfficientDiD ATT estimation, overall ATT aggregation, event-study aggregation, bootstrap inference.

• P1 Aggregated bootstrap omits the WIF / combined-IF terms for overall ATT and ES(e). The analytical path adds WIF in efficient_did.py and efficient_did.py; the new Registry section also writes extra weight terms into EIF^{es(e)} at REGISTRY.md. But the bootstrap re-aggregates perturbed cell ATTs with fixed weights in efficient_did_bootstrap.py, efficient_did_bootstrap.py, and efficient_did_bootstrap.py. The note at REGISTRY.md says this “matches” Callaway-Sant'Anna, but the repo’s own CS bootstrap uses combined IFs in staggered_bootstrap.py and staggered_bootstrap.py. Impact: aggregate bootstrap SEs/CIs/p-values can target the wrong asymptotic variance and be too tight. Concrete fix: bootstrap overall/event-study from combined IF vectors, standard aggregated IF plus WIF, instead of fixed-weight reaggregation of bootstrap_atts.

• P1 balance_e can bootstrap a different cohort set than the analytical event-study when the anchor horizon is NaN. Analytical aggregation forms groups_at_e from already finite-filtered effects in efficient_did.py and efficient_did.py. Bootstrap aggregation forms groups_at_e from all anchor-horizon (g,t) pairs in efficient_did_bootstrap.py and only drops non-finite cells later in efficient_did_bootstrap.py. Impact: the event-study point estimate and its bootstrap inference can correspond to different cohorts. Concrete fix: apply the same finite-cell filter when constructing groups_at_e in _prepare_es_agg_boot(), then add a regression test with a NaN balance_e anchor cell.

• P1 The balanced-panel validation does not reject duplicated unit-period rows, even though the Registry says the method does not handle repeated cross-sections in REGISTRY.md. The current check only counts unique periods per unit in efficient_did.py, and efficient_did.py then collapses duplicates with pivot_table(..., aggfunc="first"). Impact: invalid input can silently change ATT/SE outputs instead of failing fast. Concrete fix: reject any duplicated (unit, time) keys before pivoting.

Code Quality
No material findings beyond the methodology issues above. The new code consistently routes inference through safe_inference().

Performance
No material findings. The implementation caches slices and keeps the core linear algebra vectorized.

Maintainability
No additional material findings beyond the documentation mismatch below.

Tech Debt

• P2 The new Registry entry requires finite-sample overlap / boundary warnings in REGISTRY.md, REGISTRY.md, and REGISTRY.md, but the implementation only errors when there are zero never-treated units in efficient_did.py. Small cohort shares are inverted throughout Omega and EIF calculations in efficient_did_weights.py and efficient_did_weights.py with no warning. Impact: numerically unstable small-cohort cases will be hard to diagnose. Concrete fix: add minimum count/share warnings, and ideally a hard stop for analytical covariance/EIF calculations when any relevant cohort has fewer than 2 units.

Security
No material findings. I did not see secrets, external network calls, or risky file/command handling in the new code paths.

Documentation/Tests

• P2 EfficientDiD is now public API in init.py and init.py, but I could not find any API/tutorial docs beyond the Registry entry, and the PR-cited methodology review file docs/methodology/papers/chen-santanna-xie-2025-review.md is not present in the repo. Impact: users and future reviewers do not have a checked-in usage guide or paper audit trail for a new public estimator. Concrete fix: add an API page/tutorial and either add the paper review note or remove the stale reference.

• P2 Parameter-interaction coverage is thin in test_efficient_did.py. There is no balance_e coverage at all, and anticipation is only smoke-tested, not across event-study/group/bootstrap paths. Impact: the aggregation/inference bugs above are not being regression-tested. Concrete fix: add tests for balance_e under aggregate="event_study" and aggregate="all", including empty-after-balancing and NaN anchor cases, plus anticipation x bootstrap/group-aggregation coverage.

Static review only. I could not execute the new tests in this shell because the environment is missing project Python deps (numpy).

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 8c79b4840b9e43039ca24f27376ee55bc1b28cf3

---

Overall assessment: ⚠️ Needs changes

Executive Summary

• No P0s, but there are multiple P1 methodology issues that should block merge.
• The core PT-All cell construction and NaN gating look broadly reasonable; I did not see new inline-inference or partial-NaN anti-patterns.
• The main correctness gap is aggregated bootstrap inference: overall/event-study bootstrap skips the combined IF + WIF/share-uncertainty terms used elsewhere in the library.
• pt_assumption="post" currently goes beyond the paper-backed just-identified scope by exposing pre-treatment / negative-event output. citeturn1view0turn2view3
• Small-cohort overlap diagnostics, balance_e empty-set handling, and public docs/test coverage are still incomplete.

Methodology

• Severity: P1 Aggregated bootstrap inference is methodologically incomplete. In diff_diff/efficient_did_bootstrap.py:119 and diff_diff/efficient_did_bootstrap.py:230, overall_att and event-study bootstrap draws are built by fixed-weight re-aggregation of perturbed cell ATTs. That omits the extra weight-estimation terms in the paper’s event-study EIF, and it diverges from the existing CallawaySantAnna bootstrap, which perturbs the combined IF directly in diff_diff/staggered_bootstrap.py:291. Impact: whenever n_bootstrap > 0, aggregated SE/CI/p-values can be understated or otherwise mis-specified even if cell-level ATT bootstrap is fine. Concrete fix: compute combined aggregate EIFs (standard EIF + WIF/share terms) for overall and event-study parameters and bootstrap those directly; then update the registry text in docs/methodology/REGISTRY.md:607. citeturn2view1
• Severity: P1 pt_assumption="post" still estimates every t != period_1 cell in diff_diff/efficient_did.py:303 and aggregates negative event times in diff_diff/efficient_did.py:660. The just-identified PT-Post result in the paper is for ATT(g,t) with post-treatment t >= g, and the corollary explicitly notes PT-Post need not hold in pre-treatment periods. Impact: PT-Post users get pre-treatment outputs that are outside the paper-backed parameterization, so the estimator only “reduces to CS” on post-treatment cells, not on the full result object. Concrete fix: under PT-Post, restrict stored/aggregated EDiD effects to t >= g - anticipation, or split placebo diagnostics into a separate, explicitly labeled path with CS-style pre-period behavior. citeturn1view0turn2view3
• Severity: P1 Overlap/tiny-cohort diagnostics required by the new registry are still missing. The code immediately inverts cohort shares in Ω* and EIF calculations at diff_diff/efficient_did.py:288, diff_diff/efficient_did_weights.py:179, and diff_diff/efficient_did_weights.py:487, and the gap is even logged in TODO.md:48. Impact: very small pi_g can explode 1/pi terms and produce unstable weights/inference with no warning, despite the methodology entry requiring boundary diagnostics in docs/methodology/REGISTRY.md:488. Concrete fix: warn or error when any treated/never-treated cohort has fewer than 2 units or <1% sample share, and add regression tests for that warned path. citeturn2view4

Code Quality

• Severity: P1 balance_e has a silent empty-result path. If no cohort survives the anchor horizon, _aggregate_event_study in diff_diff/efficient_did.py:703 and _prepare_es_agg_boot in diff_diff/efficient_did_bootstrap.py:247 just return empty dicts. Impact: downstream code sees a “successful” fit with no event-study rows instead of an explicit warning/NaN structure; this is the exact empty-result edge case called out in the review checklist. Concrete fix: detect not groups_at_e, emit a warning, and return an explicit empty/NaN event-study result that downstream consumers can distinguish from a valid but short panel.

Performance

• No material findings limited to this diff.

Maintainability

• Severity: P2 The new methodology registry now enshrines the wrong bootstrap invariant. docs/methodology/REGISTRY.md:607 says fixed-weight aggregation matches CallawaySantAnna, but diff_diff/staggered_bootstrap.py:291 shows the canonical library pattern is combined IF + WIF perturbation. Impact: future contributors will preserve the current inference bug because the project’s methodology source says it is correct. Concrete fix: update the registry once the bootstrap implementation is fixed and cross-reference the CS bootstrap function rather than describing the pattern abstractly.

Tech Debt

• No additional material findings beyond the methodology/maintainability issues above.

Security

• No material findings in the changed files.

Documentation/Tests

• Severity: P2 The estimator is exported as public API in diff_diff/__init__.py:131 and diff_diff/__init__.py:240, but end-user docs/tutorial coverage is still explicitly outstanding in TODO.md:49. The new suite in tests/test_efficient_did.py:245, tests/test_efficient_did.py:731, and tests/test_efficient_did.py:957 checks post-treatment matching and finite SEs, but it does not pin the two methodology-sensitive behaviors above: combined-IF bootstrap aggregation and PT-Post negative-event suppression. Impact: the public contract for the estimator remains under-specified and the current issues can regress silently. Concrete fix: add a short estimator guide plus regression tests that fail if aggregated bootstrap stops using combined IF or if PT-Post emits unsupported pre-treatment cells.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 9dcd753eeabf5c6a1fd157df532b9645388f4615

---

Overall Assessment
⛔ Blocker

Executive Summary

• The PT-All moment construction is not paper-faithful: the implementation creates synthetic g' = np.inf indexed moments and drops the paper’s g' = g, t_pre = 1 moment, so the resulting Omega*, weights, and ATT(g,t) are not the Chen-Sant'Anna-Xie PT-All estimator. citeturn11view0turn10view0turn6view2
• Aggregated bootstrap inference is internally inconsistent: the analytical path adds WIF-style weight uncertainty for event-study aggregation, but the bootstrap path reaggregates fixed weights instead of bootstrapping the combined IF.
• Overlap / tiny-cohort diagnostics are missing even though the method assumes strictly positive group propensities and repeatedly inverts cohort shares. citeturn3view3
• The test suite currently locks in the disputed PT-All enumeration, so a correct methodology fix will require coordinated test updates.
• The estimator is now publicly exported, but there is still no API/tutorial coverage beyond the registry entry.

Methodology

• Severity: P0. Impact: diff_diff/efficient_did_weights.py#L72, diff_diff/efficient_did_weights.py#L87, diff_diff/efficient_did_weights.py#L408, and diff_diff/efficient_did_weights.py#L517 implement PT-All by indexing moments with g' = np.inf and by skipping period_1 for every cohort. The source theorem instead stacks moments over treated cohorts g1 ∈ G_trt, keeps t_pre = 1 when g1 = g, and excludes the first period only when g1 != g. That means the PR duplicates the baseline-1 never-treated moment once per t_pre and omits a valid source moment, which changes Omega*, the efficient weights, and potentially ATT(g,t). Concrete fix: enumerate only treated g' cohorts, include (g, period_1) for the same-cohort single-baseline moment, and remove the synthetic g' = np.inf indexed branch from enumeration/generated outcomes/EIF/Omega*. citeturn11view0turn10view0turn6view2
• Severity: P1. Impact: the analytical event-study path adds a WIF correction in diff_diff/efficient_did.py#L744, and the registry records extra weight terms in EIF^{es(e)} at docs/methodology/REGISTRY.md#L590, but the bootstrap path reaggregates perturbed cell ATTs with fixed weights in diff_diff/efficient_did_bootstrap.py#L137. The same shortcut is used for the library’s overall_att summary in diff_diff/efficient_did_bootstrap.py#L119. That can understate aggregated SE/CI/p-values. Concrete fix: bootstrap aggregated parameters from a combined IF/WIF vector, matching the analytical path and the existing CS bootstrap in diff_diff/staggered_bootstrap.py#L291.
• Severity: P1. Impact: validation in diff_diff/efficient_did.py#L194 checks balance and absorption, but never warns on tiny treated/control shares even though Omega* and the EIF invert those shares throughout. The paper’s overlap assumption requires strictly positive group propensities, and TODO.md#L48 already records the missing warning as deferred. Concrete fix: add a pre-estimation overlap diagnostic that warns or errors when any cohort (including never-treated) is too small for stable inversion, and surface that diagnostic in results. citeturn3view3

Code Quality

• Severity: P2. Impact: enumerate_valid_triples() accepts target_t but never uses it in diff_diff/efficient_did_weights.py#L18, and compute_eif_nocov() accepts att_gt but never uses it in diff_diff/efficient_did_weights.py#L419. In a math-heavy estimator, unused formula parameters make source drift much harder to detect. Concrete fix: remove unused parameters or add source-backed assertions/tests that force each argument to play an explicit role after the formulas are corrected.

Performance
No separate performance findings from the diff; correctness of the moment stack is the blocking issue.

Maintainability

• Severity: P2. Impact: the comments in diff_diff/efficient_did_bootstrap.py#L75 claim the fixed-weight shortcut “matches” Callaway-Sant'Anna / R did, but the local CS implementation uses the combined IF for aggregated bootstrap parameters in diff_diff/staggered_bootstrap.py#L291. That misstates the precedent and will mislead future fixes. Concrete fix: rewrite the bootstrap comments after aligning the implementation with the combined-IF path.

Tech Debt

• Severity: P2. Impact: the estimator is exported publicly in diff_diff/init.py#L132 while its own TODO list already records unresolved methodology/docs debt in TODO.md#L48 and TODO.md#L49. Concrete fix: either mark EfficientDiD experimental until those items land, or close them before merge.

Security
No findings from the diff.

Documentation/Tests

• Severity: P1. Impact: the tests in tests/test_efficient_did.py#L536, tests/test_efficient_did.py#L926, and tests/test_efficient_did.py#L1034 enforce the current g' = np.inf / skip-period_1 PT-All interpretation. A paper-faithful fix will therefore fail the suite unless these are updated in the same PR. Concrete fix: replace them with theorem-backed enumeration tests, including a positive test for g' = g, t_pre = 1 and a negative test for g' = np.inf as an indexed PT-All moment.
• Severity: P3. Impact: beyond the registry entry at docs/methodology/REGISTRY.md#L481, the PR adds no API/tutorial/README coverage for a new public estimator. That leaves users without clear guidance on the stronger PT-All default, the CS-style overall_att convention, or the fact that negative event times are placebo diagnostics rather than paper-defined ES parameters. Concrete fix: add API docs, a short tutorial, and a README entry before release. diff_diff/init.py#L132, diff_diff/efficient_did_results.py#L161 citeturn19view0

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 9dcd753eeabf5c6a1fd157df532b9645388f4615

---

Overall Assessment

⛔ Blocker

Executive Summary

• The prior PT-All blocker is still unresolved: the implementation still builds PT-All around indexed g'=∞ moments and still omits the paper’s same-group (g,1) moment, so the resulting Omega*, efficient weights, and ATT(g,t) are not the Chen-Sant’Anna-Xie PT-All estimator. citeturn8open0turn9open0turn10open0
• The prior aggregated-bootstrap blocker is also still unresolved: overall_att and event-study bootstrap inference still reaggregates fixed weights instead of bootstrapping the combined IF that includes the group-share/WIF terms. citeturn11open0turn12open0
• The small-cohort overlap warning is still missing, but the new TODO entry now tracks it, so I treat that item as P3-informational rather than blocking.
• The new tests and registry text now encode the disputed PT-All/bootstrap interpretations, so a correct methodology fix will require coordinated test and docs changes.
• The PT-Post baseline fix and NaN-safe inference path look materially improved; I did not find a new inline-inference/partial-NaN regression in the added files.

Methodology

• Severity: P0. Impact: enumerate_valid_triples() at diff_diff/efficient_did_weights.py:L18 still constructs PT-All using synthetic indexed g'=np.inf pairs and still skips period_1 for every group, while the downstream math helpers at diff_diff/efficient_did_weights.py:L112, diff_diff/efficient_did_weights.py:L344, and diff_diff/efficient_did_weights.py:L419 preserve that index set. The paper’s staggered stack keeps same-group moments (g,1),...,(g,g-1) and other-treated-group moments (g',2),...,(g',g'-1); the never-treated group enters Equation 3.9 through the separate G_∞ term, not as its own indexed g' moments. For g=2, the paper/local registry imply a 1x1 problem, but the current code synthesizes multiple moments whenever there are multiple post periods. Concrete fix: rewrite PT-All H_stg(g) to the paper form and remove the np.inf branch from enumeration, generated outcomes, Omega*, and EIF construction, then rebaseline the tests around (g,1) inclusion and g=2 => H=1. citeturn8open0turn9open0turn10open0
• Severity: P1. Impact: _run_multiplier_bootstrap() at diff_diff/efficient_did_bootstrap.py:L54 still bootstraps overall_att and event-study parameters by fixed-weight reaggregation at diff_diff/efficient_did_bootstrap.py:L119 and diff_diff/efficient_did_bootstrap.py:L137, while the analytical path adds WIF terms at diff_diff/efficient_did.py:L654 and diff_diff/efficient_did.py:L744. The paper’s efficient IF for ES(e) explicitly includes those extra group-share terms, and Section 4 says multiplier bootstrap can be applied to the estimated EIF. Current aggregated bootstrap SEs/CIs/p-values therefore omit part of the IF and can be understated. Concrete fix: build combined IF vectors (standard IF + WIF) for overall_att and each event-study horizon, then perturb those vectors directly, mirroring diff_diff/staggered_bootstrap.py:L291. citeturn11open0turn12open0

Code Quality

• No separate findings beyond the methodology blockers above.

Performance

• No separate performance findings from the diff.

Maintainability

• Severity: P2. Impact: the comments in diff_diff/efficient_did_bootstrap.py:L75 and the new registry text at docs/methodology/REGISTRY.md:L607 say the fixed-weight aggregated bootstrap matches the local Callaway-Sant’Anna pattern, but the local CS implementation uses combined IFs for overall_att and event study at diff_diff/staggered_bootstrap.py:L291. Concrete fix: update those comments/docs after the aggregated bootstrap is moved onto the combined-IF path.

Tech Debt

• Severity: P3. Impact: the finite-sample overlap warning for tiny cohort shares is still absent, but it is now explicitly tracked in TODO.md:L48. Concrete fix: none required for approval; keep the TODO until a pre-estimation small-share warning is added.

Security

• No findings.

Documentation/Tests

• Severity: P1. Impact: the new regression tests at tests/test_efficient_did.py:L533, tests/test_efficient_did.py:L940, and tests/test_efficient_did.py:L1034 now codify the disputed PT-All index set and fixed-weight aggregated bootstrap story, so a paper-faithful fix will fail the suite until these tests are rewritten. The “single pre-period” check at tests/test_efficient_did.py:L210 is also effectively vacuous because it never enforces g=2 => H=1. Concrete fix: replace them with theorem-backed tests that require (g,1) inclusion, forbid indexed g'=∞ PT-All moments, assert g=2 => H=1, and check aggregated bootstrap via combined-IF perturbation. citeturn8open0turn9open0turn10open0turn11open0turn12open0
• Severity: P3. Impact: user-facing docs/tutorial coverage for the new public estimator is now tracked in TODO.md:L49. Concrete fix: none required for approval.

Path to Approval

1. Rebuild the PT-All moment stack in diff_diff/efficient_did_weights.py:L18, diff_diff/efficient_did_weights.py:L112, diff_diff/efficient_did_weights.py:L344, and diff_diff/efficient_did_weights.py:L419 so it matches the paper’s H_stg(g): same-group moments (g,1..g-1), other treated cohorts (g',2..g'-1), and no indexed g'=∞ moments.
2. Change the aggregated multiplier bootstrap in diff_diff/efficient_did_bootstrap.py:L119, diff_diff/efficient_did_bootstrap.py:L137, and diff_diff/efficient_did_bootstrap.py:L236 to perturb combined IF vectors for overall_att and event-study outputs, modeled on diff_diff/staggered_bootstrap.py:L291.
3. Update docs/methodology/REGISTRY.md:L606, tests/test_efficient_did.py:L533, tests/test_efficient_did.py:L940, and tests/test_efficient_did.py:L1034 so the docs/tests validate the paper-faithful estimator instead of the current synthetic-moment/fixed-weight-bootstrap behavior.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 471b1ea2675037865d3efade9134b7f29c412f82

---

Overall Assessment

✅ Looks good

This re-review has no unmitigated P0/P1 findings. The two prior methodology blockers are now explicitly documented in the Methodology Registry, so under the stated review rules they are mitigated rather than blocking.

Executive Summary

• The prior PT-All index-set and aggregated-bootstrap methodology blockers are now explicitly documented in docs/methodology/REGISTRY.md:606 and docs/methodology/REGISTRY.md:607, which makes them P3-informational for this review.
• I did not find any new unmitigated P0/P1 issues in the new EfficientDiD ATT(g,t), EIF, analytical SE, or NaN-safe bootstrap/inference paths.
• One prior P2 maintainability issue remains: the new comments/registry text still say the fixed-weight aggregate bootstrap matches the current Callaway-Sant’Anna implementation, but the local CS code now uses combined IFs.
• The small-cohort overlap warning and public API docs/tutorial work are tracked in TODO.md:48 and TODO.md:49, so they are informational only.
• I did not execute the test suite in this sandbox because the available Python environment is missing numpy.

Methodology

• Severity: P3. Impact: The PT-All moment stack in diff_diff/efficient_did_weights.py:18 still departs from the paper’s staggered H_stg(g) construction, but that departure is now explicitly documented in docs/methodology/REGISTRY.md:606. Under the stated review rules, that makes it informational rather than blocking. citeturn3view0turn3view1 Concrete fix: None required for approval; only revisit if the project wants paper-faithful PT-All moments instead of the documented local convention.
• Severity: P3. Impact: The aggregate bootstrap in diff_diff/efficient_did_bootstrap.py:119 and diff_diff/efficient_did_bootstrap.py:137 still uses fixed-weight reaggregation rather than bootstrapping the full aggregated EIF. The paper’s event-study EIF includes extra group-share terms, but docs/methodology/REGISTRY.md:607 now documents this as an intentional deviation, so it is informational only for this review. citeturn4view0 Concrete fix: None required for approval unless the project decides to move back to a paper-faithful aggregate bootstrap.

Code Quality

No findings.

Performance

No findings.

Maintainability

• Severity: P2. Impact: The rationale text in diff_diff/efficient_did_bootstrap.py:119 and docs/methodology/REGISTRY.md:607 says the fixed-weight aggregate bootstrap “matches” the Callaway-Sant’Anna implementation, but the current local CS code uses combined IFs at diff_diff/staggered_bootstrap.py:291 and diff_diff/staggered_bootstrap.py:317. That stale cross-reference can mislead future reviewers and maintainers about what is actually shared across estimators. Concrete fix: Reword those comments/docs to describe this as an EfficientDiD-specific deviation from the current CS implementation, or align EfficientDiD to the same combined-IF bootstrap pattern if that comparison is meant literally.

Tech Debt

• Severity: P3. Impact: The very-small-cohort overlap/stability warning is still absent, but it is explicitly tracked in TODO.md:48. Concrete fix: None required for approval.

Security

No findings.

Documentation/Tests

• Severity: P3. Impact: User-facing API docs/tutorial coverage for the new estimator is still missing, but it is explicitly tracked in TODO.md:49. Concrete fix: None required for approval.