Skip to content

Add survey design support to EfficientDiD covariates (DR) path#264

Merged
igerber merged 6 commits intomainfrom
edid-survey-covariates
Apr 4, 2026
Merged

Add survey design support to EfficientDiD covariates (DR) path#264
igerber merged 6 commits intomainfrom
edid-survey-covariates

Conversation

@igerber
Copy link
Copy Markdown
Owner

@igerber igerber commented Apr 4, 2026

Summary

  • Thread survey weights through all five nuisance estimation steps of the EfficientDiD doubly-robust covariates path
  • Remove NotImplementedError guard that blocked covariates + survey_design combinations
  • Add 6 new tests for covariates+survey: uniform weight equivalence, scale invariance, nontrivial effect, full design smoke, aggregation, bootstrap
  • Document resolution in REGISTRY.md, survey-roadmap.md, and add Silverman bandwidth follow-up in TODO.md

Methodology references (required if estimator / math changes)

  • Method name(s): EfficientDiD (Chen, Sant'Anna & Xie, 2025) — doubly-robust covariates path with survey design
  • Paper / source link(s): Chen, X., Sant'Anna, P. H. C., & Xie, H. (2025). "Efficient Difference-in-Differences and Event Study Estimators"
  • Any intentional deviations from the source (and why): Silverman bandwidth uses unweighted statistics (survey-weighted bandwidth deferred as second-order refinement, tracked in TODO.md)

Validation

  • Tests added/updated: tests/test_survey_phase3.py — new TestEfficientDiDCovSurvey class (6 tests), replaced test_covariates_survey_raises with test_covariates_survey_works
  • Existing tests verified: 127/127 test_efficient_did.py, 63/63 test_survey_phase3.py
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

Overall Assessment

⚠️ Needs changes

Highest unmitigated severity is P1. The newly enabled EfficientDiD covariates + survey_design path mostly matches the intended weighted nuisance-estimation design, but its survey inference path is not yet methodology-consistent.

Executive Summary

  • Affected method: EfficientDiD (Chen, Sant'Anna & Xie, 2025), specifically the doubly robust covariates path under survey_design.
  • The weighted nuisance steps themselves look directionally consistent with the updated registry: WLS outcome regression, weighted sieve normal equations, survey-weighted kernel smoothing, and survey-weighted ATT averaging.
  • P1: analytical survey aggregation SEs are mismatched because EfficientDiD switches to survey-weighted cohort shares but keeps an unweighted WIF formula.
  • P1: the newly enabled covariates + survey_design + n_bootstrap path inherits a survey bootstrap that perturbs raw EIFs instead of survey-scaled IF contributions.
  • P1: zero-total-weight cohorts/comparison groups are not handled as empty weighted samples, so the new DR path can divide by zero or fall back to clipped nonsense.
  • The Silverman-bandwidth deviation is properly documented in the registry and tracked in TODO.md, so it is not a blocker.

Methodology

Code Quality

  • Severity: P1. Impact: zero-total-weight cohorts are not treated as empty weighted samples. cohort_fractions[g] can be zero in diff_diff/efficient_did.py:L575-L583, but compute_generated_outcomes_cov() still divides by pi_g in terms 2 and 3 at diff_diff/efficient_did_covariates.py:L507-L527, and estimate_inverse_propensity_sieve() can divide by sum(w_group) in diff_diff/efficient_did_covariates.py:L364-L375. That can yield NaN/Inf or arbitrary clipped values instead of a controlled skip/error. Concrete fix: validate positive weighted mass for every treated/comparison cohort before nuisance estimation, skip or raise on zero-weight groups, and add a regression test with a zero-weight treated cohort.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • Severity: P3. Impact: the unweighted Silverman-bandwidth choice is explicitly documented in docs/methodology/REGISTRY.md:L702-L706 and tracked in TODO.md:L68, so this is acceptable deferred work under the project rules. Concrete fix: none required for approval.

Security

  • No findings.

Documentation/Tests

  • Severity: P2. Impact: the new tests only check finiteness for aggregation/bootstrap in tests/test_survey_phase3.py:L924-L1045; they would not catch the two inference bugs above, and there is still no replicate-weight coverage for covariates + survey_design. Concrete fix: add one analytical survey-SE regression test under non-uniform weights, one heterogeneous-weight bootstrap regression test, and one replicate-weight analytical test for the new DR path.

  • Severity: P3. Impact: the public survey tutorial still says EfficientDiD does not support covariates + survey_design in docs/tutorials/16_survey_did.ipynb:L530, which now conflicts with docs/survey-roadmap.md:L38 and docs/methodology/REGISTRY.md:L706. Concrete fix: update the tutorial notebook’s support matrix/note.

Path to Approval

  1. Make EfficientDiD’s survey aggregation WIF match the survey-weighted formula already used elsewhere in the library, and regression-test default overall_se plus event-study SEs under heterogeneous weights.
  2. Rework EfficientDiD’s survey bootstrap to perturb survey-scaled IF contributions rather than raw EIF/n, and add a heterogeneous-weight bootstrap regression test for covariates + survey_design.
  3. Guard zero-total-weight cohorts/comparison groups as empty weighted samples and add a regression test where one treated cohort has zero unit weight.

@igerber
Copy link
Copy Markdown
Owner Author

igerber commented Apr 4, 2026

/ai-review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: 66ab529283e6570110cb661d64c03deb56d7e98d

Overall Assessment

⚠️ Needs changes

Static review only: I could not execute the added tests in this environment because pytest and numpy are unavailable.

Executive Summary

Methodology

  • Severity: P1. Impact: The previous survey aggregation-SE issue is not fully fixed. _compute_wif_contribution() now ports the survey-weighted w_i * 1{G_i=g} - p_g WIF from the staggered aggregation path (diff_diff/efficient_did.py:L1188, diff_diff/staggered_aggregation.py:L372), but EfficientDiD still computes survey SEs through compute_survey_vcov(), which expects unweighted residuals and applies w_i internally (diff_diff/efficient_did.py:L1101, diff_diff/survey.py:L1416). That leaves the overall/event-study WIF correction on the wrong survey-IF scale, so the newly enabled covariates + survey_design path can still report incorrect analytical SEs. Concrete fix: keep aggregation on one convention end to end: either compute aggregated survey SEs from a fully score-scaled psi object via compute_survey_if_variance(), or rewrite the WIF term into the residual convention compatible with compute_survey_vcov() and add heterogeneous-weight regression tests for default overall SE and event-study SE.
  • Severity: P1. Impact: The survey bootstrap fix is incomplete. _run_multiplier_bootstrap() still perturbs (all_weights @ eif_gt) / denom (diff_diff/efficient_did_bootstrap.py:L143), but the survey variance machinery is built around weighted scores (w_i * residual_i) (diff_diff/survey.py:L1432). So the new covariates + survey_design + n_bootstrap>0 path still ignores the base survey weights in the perturbed influence object, which means bootstrap SEs, CIs, and p-values are not targeting the same design-based variance as the analytical path. Concrete fix: bootstrap the same survey-score object used for survey inference, i.e. unit-level w_i * EIF_i / sum(w) or PSU-aggregated survey scores, not raw EIF_i / sum(w), and add a heterogeneous-weight bootstrap regression test that fails if survey weights are dropped.

Code Quality

  • Severity: P1. Impact: Zero-weight handling is still incomplete in the newly enabled DR path. The new guard only skips target cohorts with zero survey mass (diff_diff/efficient_did.py:L678), but weighted outcome regression is still called on comparison groups and never-treated controls without checking whether their total survey weight is zero (diff_diff/efficient_did.py:L737, diff_diff/efficient_did.py:L751). In that case solve_ols() raises on an empty effective sample (diff_diff/efficient_did_covariates.py:L75, diff_diff/linalg.py:L405), and estimate_inverse_propensity_sieve() still divides by sum(w_group) in its fallback path (diff_diff/efficient_did_covariates.py:L365). Concrete fix: validate positive total survey weight for every comparison group used by each (g,t) cell before nuisance estimation; if a required comparison group has zero effective weight, skip that cell or raise a clear identification error instead of relying on downstream solver failures or clipped fallback values.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • Severity: P3. Impact: The unweighted Silverman bandwidth choice is explicitly documented in the methodology registry and tracked in TODO.md, so under the project’s review rules this is acceptable deferred work, not a defect (docs/methodology/REGISTRY.md:L706, TODO.md:L68). Concrete fix: none required for approval.

Security

  • No findings.

Documentation/Tests

  • Severity: P2. Impact: The new tests are mostly smoke checks and broad plausibility checks. They verify finiteness and coarse bootstrap/analytical agreement, but they do not pin the survey aggregation IF scale, the survey bootstrap score scale, or an actual zero-weight effective comparison group (tests/test_survey_phase3.py:L1011, tests/test_survey_phase3.py:L1032, tests/test_survey_phase3.py:L1068). The new “zero-weight cohort” test uses 1e-15, not zero, so it never exercises the new zero-weight skip branch (tests/test_survey_phase3.py:L1090). Concrete fix: add exact regression tests for heterogeneous-weight analytical overall/event-study SEs, survey-aware bootstrap perturbations, and a true zero-weight never-treated or comparison cohort.

Path to Approval

  1. Rework EfficientDiD’s survey aggregation inference so the WIF correction and the survey variance helper use the same IF convention end to end; then add regression tests for default overall_se and aggregate="event_study" under non-uniform weights.
  2. Rework the survey bootstrap to perturb survey-score contributions, not raw eif_gt, and add a heterogeneous-weight bootstrap regression test for covariates + survey_design.
  3. Add explicit effective-weight-mass validation for all nuisance-estimation comparison groups, and add a test where the never-treated group or a comparison cohort has exactly zero survey weight.

@igerber
Copy link
Copy Markdown
Owner Author

igerber commented Apr 4, 2026

/ai-review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: 265a1bc3af44a979d1873b85725251b734829a7f

Overall Assessment

⚠️ Needs changes

Static review only: I did not execute the tests in this environment because the local Python environment is missing numpy/pytest.

Executive Summary

Methodology

  • Severity: P1. Impact: The new aggregation SE branch is incorrect for replicate-weight survey designs. _compute_survey_eif_se() still correctly dispatches replicate designs to compute_replicate_if_variance() (diff_diff/efficient_did.py:L1126-L1136), but _aggregate_overall() and _aggregate_event_study() now call compute_survey_if_variance() unconditionally whenever a survey design is present (diff_diff/efficient_did.py:L1277-L1288, diff_diff/efficient_did.py:L1386-L1393). On replicate designs, compute_survey_if_variance() ignores replicate weights and falls into the implicit-PSU TSL path instead (diff_diff/survey.py:L1202-L1219), whereas the correct replicate contract is implemented in compute_replicate_if_variance() (diff_diff/survey.py:L1682-L1735). That silently changes supported replicate-weight inference for overall_se and aggregate="event_study", which is an undocumented variance-methodology mismatch against the EfficientDiD survey support described in the registry (docs/methodology/REGISTRY.md:L700-L706). Concrete fix: dispatch psi_total through the same replicate-vs-TSL branching used elsewhere, or reuse the staggered aggregation pattern directly (diff_diff/staggered_aggregation.py:L500-L528).
  • Severity: P1. Impact: The newly enabled DR survey path still lacks an identification check for a zero-survey-mass never-treated control group. The code warns when the never-treated cohort fraction is zero (diff_diff/efficient_did.py:L609-L616) but continues into nuisance estimation, and every valid DR pair still requires m_hat_{∞,t,tpre}(X) (diff_diff/efficient_did.py:L752-L762). That path passes all-zero weights into solve_ols() via estimate_outcome_regression() (diff_diff/efficient_did_covariates.py:L69-L84), which raises “Weights sum to zero” on an empty effective sample (diff_diff/linalg.py:L395-L409). Under the review rubric, this is still a missing assumption/edge-case check on the PR’s new supported path. Concrete fix: fail fast with a clear identification error, or skip affected (g,t) cells before any m_hat_{∞,...} / r_{g,∞} estimation when the never-treated group has zero total survey weight.

Code Quality

  • No additional findings beyond the methodology/edge-case issues above.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • Severity: P3. Impact: The unweighted Silverman bandwidth choice is explicitly documented in the Methodology Registry and tracked in TODO.md, so this is acceptable deferred work under the project’s rules, not a blocker (docs/methodology/REGISTRY.md:L706-L706, TODO.md:L68-L68). Concrete fix: none required for approval.

Security

  • No findings.

Documentation/Tests

  • Severity: P2. Impact: The new tests cover the non-replicate pweight path and a zero-weight treated cohort, but they do not exercise either of the changed failure surfaces above: replicate-weight aggregation inference or an exact zero-weight never-treated control group. The new coverage is concentrated in tests/test_survey_phase3.py:L924-L1108, while existing EfficientDiD replicate coverage only checks that the default result is finite, not that aggregation uses the replicate-weight variance path (tests/test_survey_phase6.py:L1196-L1210). Concrete fix: add one replicate-weight regression test that asserts overall_se and aggregate="event_study" stay on the replicate path, and one covariates+survey test where the never-treated cohort has exactly zero weight and the estimator fails early with a clear message.

Path to Approval

  1. Restore replicate-weight handling in EfficientDiD aggregation SEs by branching on uses_replicate_variance inside _aggregate_overall() and _aggregate_event_study(), and pass psi_total to compute_replicate_if_variance() instead of compute_survey_if_variance() for replicate designs.
  2. Add an explicit DR-path identification guard for zero-survey-mass never-treated controls before any nuisance estimation, so covariates + survey_design raises a clear error or deterministically skips affected cells instead of falling into solve_ols() with all-zero weights.
  3. Add regression tests for both cases: replicate-weight EfficientDiD aggregation SEs, and covariates+survey with the never-treated group set to exactly zero weight.

@igerber
Copy link
Copy Markdown
Owner Author

igerber commented Apr 4, 2026

/ai-review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: cc0e65916634f0496d6613dff8cbf32298e7b1c6

Overall Assessment

✅ Looks good

Static review only: pytest is not installed in this environment, so I reviewed the diff and surrounding code but did not execute the test suite.

Executive Summary

  • Re-review scope: the two prior P1s appear resolved. The DR survey path now fails fast on zero-survey-mass never-treated controls in efficient_did.py:L609, and overall/event-study survey aggregation now routes replicate designs back through compute_replicate_if_variance() in efficient_did.py:L1279 and efficient_did.py:L1395.
  • Affected method: EfficientDiD’s staggered-adoption DR covariate path. The changed code still follows the paper’s sieve-ratio / inverse-propensity / kernel-smoothed Omega* / generated-outcome structure for Eqs. 4.2, 3.12, 4.4, and 4.3, with plug-in EIF logic justified by the paper’s Neyman-orthogonal construction. (arxiv.org)
  • I did not find any new unmitigated P0/P1 correctness issues in the changed estimator math.
  • The main remaining issue is non-blocking test coverage: the PR still does not add a replicate-weight regression test for the exact aggregation SE path that was previously wrong.
  • The Silverman-bandwidth choice is explicitly documented in the registry and tracked in TODO.md, so it is mitigated and informational only.

Methodology

Previously raised methodology blockers look fixed in the changed code: the estimator now guards the zero-weight never-treated control case before nuisance estimation in efficient_did.py:L609, and replicate-weight aggregation SEs no longer fall back to the wrong survey-variance helper in efficient_did.py:L1279 and efficient_did.py:L1395.

  • Severity: P3. Impact: The newly enabled survey-weighted DR path is a documented extension in the Methodology Registry, not an undocumented drift from source material; the registry now explicitly records WLS outcome regression, weighted sieve normal equations, survey-weighted kernel smoothing, and survey-weighted ATT averaging in REGISTRY.md:L706, with the implementation threaded through efficient_did.py:L744, efficient_did_covariates.py:L152, and efficient_did_covariates.py:L623. Concrete fix: none required.

Code Quality

  • No findings.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • Severity: P3. Impact: _silverman_bandwidth() remains unweighted in efficient_did_covariates.py:L545, but that limitation is explicitly documented in REGISTRY.md:L706 and tracked in TODO.md:L68. Under the review rules, this is properly mitigated deferred work. Concrete fix: none required for approval.

Security

  • No findings.

Documentation/Tests

  • Severity: P2. Impact: The PR adds useful coverage for weights-only/full-design/bootstrap/zero-weight covariate-survey paths in test_survey_phase3.py:L924, but it still does not lock down the exact replicate-weight aggregation regression that was previously broken. Existing replicate coverage in test_survey_phase6.py:L1196 is still a finite-result smoke test, so a future fallback away from the replicate helper inside _aggregate_overall() or _aggregate_event_study() could recur without being caught. Concrete fix: add a replicate-weight EfficientDiD regression with aggregate="event_study" or aggregate="all" that specifically exercises efficient_did.py:L1279 and efficient_did.py:L1395.

igerber and others added 5 commits April 4, 2026 12:19
@igerber @claude
Add survey design support to EfficientDiD covariates (DR) path
2205914 
Thread survey weights through all five nuisance estimation steps of the
doubly-robust covariates path: WLS outcome regression, weighted sieve
normal equations for propensity ratios/inverse propensities, survey-weighted
Nadaraya-Watson kernel for conditional Omega*(X), and survey-weighted ATT
averaging. Remove the NotImplementedError guard that blocked covariates +
survey_design combinations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@igerber @claude
Address P1/P2 review findings: survey-weighted WIF, bootstrap denomin…
0a12a77 
…ator, zero-weight guard

- Make _compute_wif_contribution() survey-aware: use w_i * 1{G_i=g} - pg_k
  formula when unit_weights present, matching staggered_aggregation.py
- Use explicit sum(unit_level_weights) denominator in bootstrap perturbation
  when survey design is active
- Guard zero-weight cohorts: skip in fit loop, early return in
  compute_generated_outcomes_cov when pi_g <= 0
- Add regression tests: analytical SE differs from unweighted, bootstrap SE
  in ballpark of analytical, zero-weight cohort handled gracefully
- Update tutorial notebook: remove stale note about covariates+survey

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@igerber @claude
Fix IF-scale mismatch in survey aggregation, bootstrap scores, zero-w…
aca9f8b 
…eight guards

- Switch aggregated survey SEs from compute_survey_vcov() to
  compute_survey_if_variance() with score-level psi values, avoiding
  double-weighting of the survey-weighted WIF term
- Bootstrap now perturbs survey-score object w_i*eif_i/sum(w) instead
  of raw eif_i/n, matching analytical variance convention
- Filter zero-weight comparison groups from valid pairs before nuisance
  estimation; guard in estimate_inverse_propensity_sieve fallback
- Fix zero-weight test to use actual 0.0 weights with pytest.warns

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@igerber @claude
Restore replicate-weight dispatch in aggregation, raise on zero-weigh…
42d3dff 
…t never-treated

- Add uses_replicate_variance branching in _aggregate_overall() and
  _aggregate_event_study() to route replicate designs to
  compute_replicate_if_variance() instead of compute_survey_if_variance()
- Change zero-weight never-treated guard from warning to ValueError for
  covariates path — DR nuisance estimation requires positive-weight controls
- Add test_zero_weight_never_treated_raises for the new error path

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@igerber @claude
Add replicate-weight aggregation regression test for DR+survey path
d45df25 
Exercises the compute_replicate_if_variance dispatch in _aggregate_overall
and _aggregate_event_study with covariates + replicate-weight survey design,
guarding against future regressions to the wrong variance helper.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@igerber igerber force-pushed the edid-survey-covariates branch from cc0e659 to d45df25 Compare April 4, 2026 16:25
@igerber
Copy link
Copy Markdown
Owner Author

igerber commented Apr 4, 2026

/ai-review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: d45df253a0eb3f71f67dfc640ddf8ab92308cbae

Overall Assessment

⚠️ Needs changes

Static review only: the workspace Python environment is missing numpy, so I could not execute the added tests.

Executive Summary

  • Affected method: EfficientDiD’s staggered-adoption survey-aware path, especially the newly enabled DR covariates branch plus the survey aggregation / bootstrap plumbing.
  • Methodology check: the core estimator still follows the paper’s staggered generated-outcome, conditional Omega*, efficient ATT, and cohort-share event-study structure; the survey-weighted nuisance threading is explicitly documented in the registry, so I do not treat that extension itself as a defect. (psantanna.com)
  • The prior re-review blockers appear resolved: the DR path now fails fast on zero-weight never-treated controls in diff_diff/efficient_did.py#L609, and aggregated survey SEs now dispatch replicate designs through the replicate IF helper in diff_diff/efficient_did.py#L1288 and diff_diff/efficient_did.py#L1404.
  • [Newly identified] The zero-weight never-treated fix is only applied to use_covariates=True; the no-cov survey path still appears able to fall through to a zero-sum weighted mean instead of raising a controlled library error.
  • The Silverman-bandwidth refinement is properly documented in docs/methodology/REGISTRY.md#L706 and tracked in TODO.md#L72, so it is mitigated.
  • Documentation is internally inconsistent: docs/survey-roadmap.md#L39 marks EfficientDiD covariates+survey as resolved, but docs/survey-roadmap.md#L305 still says it is unsupported.

Methodology

  • Severity: P3. Impact: The changed estimator logic is EfficientDiD’s staggered-adoption covariates path. The source paper’s staggered estimator is still built from the Eq. 4.4 generated outcomes, Eq. 3.12 conditional Omega*, Eq. 4.3 efficient ATT construction, and Eq. 4.5 cohort-share event-study aggregation, and the PR’s survey-weighted WLS/sieve/kernel/ATT changes are explicitly documented as a registry note in docs/methodology/REGISTRY.md#L706. Concrete fix: none required. (psantanna.com)
  • Severity: P1 [Newly identified]. Impact: The new zero-weight never-treated guard in diff_diff/efficient_did.py#L609 is gated by and use_covariates, but the no-cov survey path still computes a weighted never-treated mean via diff_diff/efficient_did_weights.py#L447 and diff_diff/efficient_did_weights.py#L470. With all never-treated survey mass at zero, EfficientDiD(..., survey_design=sd, covariates=None) will hit a low-level zero-sum weighted-mean failure instead of a controlled estimator ValueError. This is a partial fix of the same zero-weight comparison-group pattern. Concrete fix: move the never-treated positive-weight validation out of the DR-only branch, or add an equivalent guard inside the no-cov generated-outcome path before calling the weighted mean.

Code Quality

  • No findings.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

Security

  • No findings.

Documentation/Tests

Path to Approval

  1. Extend the zero-weight never-treated validation to the no-cov survey path so that EfficientDiD(..., survey_design=sd, covariates=None) fails fast with a library ValueError instead of reaching a zero-sum weighted mean.
  2. Add a regression test for that no-cov zero-weight never-treated scenario in tests/test_survey_phase3.py.

@igerber @claude
Extend zero-weight never-treated guard to nocov path, fix roadmap inc…
ca9522c 
…onsistency

- Remove `and use_covariates` condition from zero-weight never-treated
  guard so both DR and nocov survey paths fail fast with ValueError
- Remove stale covariates+survey limitation entry from survey-roadmap.md
  deferred list (now resolved)
- Add test_zero_weight_never_treated_nocov_raises for nocov path

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@igerber
Copy link
Copy Markdown
Owner Author

igerber commented Apr 4, 2026

/ai-review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: ca9522c4c204a48838c46d327a0856026e88efc3

Overall Assessment
✅ Looks good

Static review only: I could not execute tests in this workspace because pytest, numpy, pandas, and scipy are not installed.

Executive Summary

Methodology

  • Severity: P3. Impact: The PR extends EfficientDiD’s covariate-adjusted DR path to survey designs, but that extension is explicitly documented in docs/methodology/REGISTRY.md:L705-L706, and the remaining Silverman-bandwidth refinement is already tracked in TODO.md:L72-L72. Cross-checking the changed estimator/variance code did not surface an undocumented deviation in estimator construction, weighting, identification logic, or SE handling. Concrete fix: none required.

Code Quality

  • No findings.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • No findings.

Security

  • No findings.

Documentation/Tests

  • Severity: P3. Impact: The tutorial notebook now documents replicate methods as only JK1, JKn, BRR, and Fay, but SDR support remains documented and tested elsewhere in docs/survey-roadmap.md:L96-L96 and tests/test_survey_phase8.py:L92-L242. That understates supported functionality for users reading the tutorial. Concrete fix: restore SDR in the notebook’s replicate-method list and summary bullet, or add an explicit note explaining why the tutorial intentionally excludes it.

@igerber igerber merged commit 1f6ade8 into main Apr 4, 2026
14 checks passed
@igerber igerber deleted the edid-survey-covariates branch April 4, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@igerber