Add survey Phase 6: replicate weights, DEFF diagnostics, subpopulation analysis

[igerber]

Summary

• Add replicate weight variance (BRR, Fay's BRR, JK1, JKn) as alternative to TSL
• Add per-coefficient DEFF diagnostics comparing survey vs SRS variance
• Add subpopulation analysis via SurveyDesign.subpopulation()
• Fix EfficientDiD hausman_pretest() stale n_cl after NaN filtering
• Fix ContinuousDiD event-study anticipation filter
• Extract _format_survey_block() helper across 11 results files
• Rename DEFF display label to "Kish DEFF (weights)"
• Add ResolvedSurveyDesign.subset_to_units() for panel→unit collapse with replicate metadata
• Add zero-weight guards in solve_ols(), solve_logit(), and estimator cell means
• Reject replicate+bootstrap combinations in CS, ContinuousDiD, EfficientDiD, SunAbraham
• Guard Bacon decomposition weighted cell means against zero effective weight
• 15 commits across 14 rounds of AI review with gpt-5.4-pro

Methodology references (required if estimator / math changes)

• Method name(s): Replicate Weight Variance (BRR, Fay, JK1, JKn), DEFF Diagnostics, Subpopulation Analysis
• Paper / source link(s): Wolter (2007) "Introduction to Variance Estimation"; Rao & Wu (1988) JASA 83(401); Kish (1965) "Survey Sampling"; Lumley (2004) JSS 9(8)
• Any intentional deviations from the source (and why): SunAbraham rejects replicate-weight designs (weighted within-transformation must be recomputed per replicate — not yet implemented). ContinuousDiD/EfficientDiD/CS reject replicate weights + n_bootstrap>0 (replicate variance is analytical, not bootstrap-compatible).

Validation

• Tests added/updated: tests/test_survey_phase6.py (53 new tests), tests/test_survey_phase3.py, tests/test_survey_phase4.py, tests/test_survey_phase5.py (coverage gap tests), tests/test_survey.py, tests/test_efficient_did.py, tests/test_continuous_did.py
• Numerical validation: Replicate IF variance matches TSL IF variance within 0.3% on toy weighted PSU design
• Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

Overall Assessment
⚠️ Needs changes

Executive Summary

• P1: SurveyDesign.subpopulation() does not actually validate that mask is boolean; object/string masks are coerced with astype(bool), which can silently define the wrong domain.
• P1: the new zero-weight branch in solve_logit() drops columns on the positive-weight subset but never expands the returned coefficient vector back to the original feature layout, breaking the solver contract and cached propensity-score reuse.
• The replicate-variance scale choices I checked against the new Phase 6 registry entry are internally consistent for CS aggregation, ContinuousDiD, EfficientDiD, and TripleDifference.
• I did not find new inline inference anti-patterns or partial NaN-gating problems in the changed inference paths.
• One methodology-doc gap remains: estimator-level replicate limitations are in the roadmap, but not yet in the canonical Methodology Registry.

Methodology

• Severity P1 Impact: the new domain-estimation API can silently target the wrong subpopulation. SurveyDesign.subpopulation() only rejects float NaN and literal None, then does raw_mask.astype(bool). For object/string-coded masks, non-empty strings become True, so excluded observations can be retained with no warning. That changes the estimand for the new subpopulation feature rather than failing fast. Evidence: diff_diff/survey.py:L412-L435, docs/methodology/REGISTRY.md:L2044-L2057. Concrete fix: require a true boolean mask (or an explicitly allowed {0,1} numeric mask), reject string/object masks and pd.NA, and add regression tests for string-coded domain columns.

Code Quality

• Severity P1 Impact: the new zero-weight rank check in solve_logit() mutates X_with_intercept and shrinks k when the positive-weight subset is rank-deficient, but the return path only re-expands columns dropped by the second rank check. The result is a shortened coefficient vector instead of the original p+1 layout. That breaks solve_logit()’s return contract and can fail in production when cached propensity coefficients are reused via X_all_with_intercept @ beta_logistic in CS IPW/DR. Evidence: diff_diff/linalg.py:L1189-L1233, diff_diff/linalg.py:L1331-L1338, diff_diff/staggered.py:L1804-L1811. Concrete fix: preserve the original column count, union the dropped-column sets from both rank checks, and always expand the returned coefficient vector back to the original p+1 positions before returning/caching it.

Performance

• No findings.

Maintainability

• No findings beyond the test/doc gaps below.

Tech Debt

• No findings. The TODO.md updates are resolution bookkeeping for previously tracked review items, not new untracked debt.

Security

• No findings.

Documentation/Tests

• Severity P3 Impact: the estimator-level replicate limitations are documented only in the roadmap. docs/survey-roadmap.md records that SunAbraham rejects replicate-weight designs and CS/ContinuousDiD/EfficientDiD reject replicate weights with bootstrap, but the new Phase 6 registry section does not, so the canonical methodology registry is incomplete for these exceptions. Evidence: docs/survey-roadmap.md:L103-L106, docs/methodology/REGISTRY.md:L1996-L2024. Concrete fix: add **Note:** entries in REGISTRY.md for these estimator-level limitations.
• Severity P2 Impact: the new tests miss both blockers above. The added solve_logit tests stop at warning/error behavior and do not assert full-length coefficient expansion, and the subpopulation tests cover NaN/None masks but not non-boolean string/object masks. Evidence: tests/test_survey_phase6.py:L715-L747, tests/test_survey_phase6.py:L953-L970. Concrete fix: add one regression test that asserts solve_logit() returns a p+1-length beta under zero-weight rank deficiency, and one that passes a string/object mask column and expects ValueError.

Path to Approval

1. Fix solve_logit() so positive-weight-subset column dropping preserves the original coefficient indexing/shape, then add a regression test that exercises cached propensity-score reuse after zero-weight subpopulation filtering.
2. Make SurveyDesign.subpopulation() reject non-boolean masks before coercion, including string/object masks and nullable-boolean pd.NA, then add regression tests for those cases.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 200745cc60011c7349f5ac2fa7d6050bc225bcee

---

Overall Assessment

⚠️ Needs changes

Executive Summary

• The two prior blockers from the last AI review appear addressed: SurveyDesign.subpopulation() now rejects string/object masks, and solve_logit() re-expands coefficients to the original layout after positive-weight rank checks. See diff_diff/survey.py:L428-L446 and diff_diff/linalg.py:L1189-L1348.
• P1: the new subpopulation API still silently accepts non-binary numeric masks via astype(bool), so coded domain columns like {1,2} or {1,2,3} can estimate the wrong subpopulation with no warning. See diff_diff/survey.py:L422-L447 and the Phase 6 subpopulation registry entry at docs/methodology/REGISTRY.md:L2047-L2064.
• P1: replicate-weight variance now drops invalid replicates for the variance calculation, but the reported survey d.f. remains R-1 from the original design. That makes t-based p-values/CIs stale whenever replicate solves or replicate contrasts are discarded. See diff_diff/survey.py:L529-L532, diff_diff/survey.py:L1291-L1314, diff_diff/survey.py:L1390-L1414, and inference consumers such as diff_diff/linalg.py:L1855-L1860 and diff_diff/linalg.py:L2017-L2036.
• The methodology-registry alignment is materially better in this revision: the Phase 6 registry now documents SunAbraham’s replicate-weight rejection and the CS/ContinuousDiD/EfficientDiD replicate-plus-bootstrap rejection, so those are no longer blockers. See docs/methodology/REGISTRY.md:L2026-L2031.
• Test coverage was added for the previous blockers, but I did not find regression tests for non-binary numeric masks or for d.f. updates after invalid replicate drops. See tests/test_survey_phase6.py:L127-L175, tests/test_survey_phase6.py:L754-L760, and tests/test_survey_phase6.py:L399-L499.

Methodology

• Severity: P1. Impact: SurveyDesign.subpopulation() still does not enforce the “boolean or 0/1 mask” contract; after the string/None checks it coerces all remaining numeric masks with raw_mask.astype(bool), so a coded domain variable like {1,2} becomes all-True and silently targets the wrong estimand. For a survey-domain feature, that is a missing assumption check on the new methodology path. Evidence: diff_diff/survey.py:L422-L447, docs/methodology/REGISTRY.md:L2047-L2064. Concrete fix: accept only real boolean masks or numeric masks whose non-missing unique values are a subset of {0,1}; reject all other numeric codes and nullable-boolean pd.NA with ValueError.
• Severity: P1. Impact: replicate-weight inference uses stale survey degrees of freedom after invalid replicates are discarded. compute_replicate_vcov() and compute_replicate_if_variance() explicitly drop failed/invalid replicates and recompute the variance from the surviving subset, but ResolvedSurveyDesign.df_survey is still fixed at n_replicates - 1, and that stale d.f. is then fed into safe_inference() on changed paths. This can understate p-values / narrow CIs when some replicates are unusable, which is an inference-methodology error, not just a presentation issue. Evidence: diff_diff/survey.py:L529-L532, diff_diff/survey.py:L1291-L1314, diff_diff/survey.py:L1390-L1414, diff_diff/linalg.py:L1855-L1860, diff_diff/linalg.py:L2017-L2036, docs/methodology/REGISTRY.md:L2009-L2025. Concrete fix: return the valid replicate count from the replicate-variance helpers, carry it through estimator fit/inference, and use n_valid - 1 as the survey d.f. whenever replicates were dropped.

Code Quality

• No new findings. The prior solve_logit() coefficient-shape regression appears fixed in diff_diff/linalg.py:L1193-L1348.

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• No findings. The PR properly resolves prior tracked items in TODO.md, including the EfficientDiD Hausman cluster-count fix, the ContinuousDiD anticipation filter, survey metadata formatting dedup, and survey bootstrap coverage. See TODO.md:L55-L63 and TODO.md:L77-L81.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: the new tests do not cover either unmitigated methodology issue above. Subpopulation tests cover boolean, string, NaN, and None, but not non-binary numeric masks; replicate tests assert df_survey == len(rep_cols) - 1, but there is no regression for partial replicate failure updating inference d.f. Evidence: tests/test_survey_phase6.py:L127-L175, tests/test_survey_phase6.py:L754-L760, tests/test_survey_phase6.py:L399-L499. Concrete fix: add one test that passes a numeric-coded mask like {1,2} and expects ValueError, and one test that forces some replicate solves/contrasts invalid and asserts the reported d.f. matches the number of valid replicates rather than the original R.

Path to Approval

1. Tighten SurveyDesign.subpopulation() so it accepts only boolean masks or numeric 0/1 masks, and rejects non-binary numeric codes plus nullable-boolean missing values with a deterministic ValueError.
2. Thread effective valid-replicate counts through the replicate-variance helpers and estimator inference paths, and use n_valid - 1 for replicate-design t-based inference whenever any replicates are dropped.
3. Add regression tests for the two cases above: a non-binary numeric subpopulation mask, and a replicate-design fit where some replicates are invalid but inference remains finite.

I did not execute the test suite in this sandbox because the available Python environment is missing project dependencies (numpy was unavailable).

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: a8b3e4c6ab68fb66743531d480cc0829b34d445a

---

Overall Assessment

⛔ Blocker

Executive Summary

• The prior subpopulation-mask blocker appears fixed: SurveyDesign.subpopulation() now rejects non-binary numeric masks before boolean coercion, with a matching regression test in diff_diff/survey.py:L447-L456 and tests/test_survey_phase6.py:L762-L769.
• The documented replicate/bootstrapping deviations are now aligned with the Methodology Registry, so those are no longer defects under this rubric; see docs/methodology/REGISTRY.md:L2023-L2031.
• P0 [Newly identified]: CallawaySantAnna still drops replicate metadata when collapsing survey designs to unit level, so replicate-weight fits silently fall back to non-replicate aggregation SEs/metadata/d.f. instead of the registered replicate-IF variance path; see diff_diff/staggered.py:L332-L391, diff_diff/staggered.py:L464-L486, diff_diff/staggered_aggregation.py:L473-L497, and docs/methodology/REGISTRY.md:L2009-L2025.
• P1: invalid replicates are dropped for variance calculation, but df_survey remains fixed at R-1, so t-based p-values/CIs use stale d.f. whenever replicate solves or replicate contrasts are discarded; see diff_diff/survey.py:L538-L541, diff_diff/survey.py:L1300-L1333, diff_diff/survey.py:L1399-L1408, diff_diff/linalg.py:L1854-L1860, diff_diff/continuous_did.py:L1284-L1293, and diff_diff/efficient_did.py:L519-L524.
• Phase 6 tests cover happy paths, but they still do not pin either live blocker: CS replicate dispatch during unit collapse, or n_valid-1 d.f. after invalid replicate drops; see tests/test_survey_phase6.py:L399-L411, tests/test_survey_phase6.py:L486-L499, and tests/test_survey_phase6.py:L850-L865.

Methodology

• Severity: P0 [Newly identified]. Impact: the new Phase 6 replicate-weight methodology is not actually preserved in CallawaySantAnna aggregation. _collapse_survey_to_unit_level() rebuilds a plain ResolvedSurveyDesign with only weights/strata/psu/fpc, dropping replicate_weights, replicate_method, fay_rho, replicate_strata, and n_replicates. The aggregation code then checks precomputed["resolved_survey_unit"]; because replicate metadata is gone, it skips compute_replicate_if_variance() and falls through to the non-replicate np.sum(psi_total**2) path, while reported survey metadata/d.f. are also recomputed as if the design were ordinary weights-only. That is silent wrong statistical output for replicate-weight CS fits, and it contradicts the registered “CS aggregation” replicate-IF method. Evidence: diff_diff/staggered.py:L332-L391, diff_diff/staggered.py:L464-L486, diff_diff/staggered.py:L1361-L1374, diff_diff/staggered_aggregation.py:L473-L497, docs/methodology/REGISTRY.md:L2009-L2025. Concrete fix: replace the bespoke unit-collapse constructor with the new replicate-aware helper (subset_to_units() or equivalent), preserve replicate metadata in resolved_survey_unit, and recompute CS overall/event-study/group SEs, metadata, and survey d.f. from that replicate-aware unit design.
• Severity: P1. Impact: the previous d.f. issue is still unresolved. compute_replicate_vcov() and compute_replicate_if_variance() both drop invalid replicates and compute variance from the surviving subset, but ResolvedSurveyDesign.df_survey is still hard-coded to n_replicates - 1. Every inference path that reads df_survey therefore keeps using stale t-distribution d.f. when some replicates fail, which makes p-values/CIs inconsistent with the variance actually reported. The new TODO entry does not mitigate this under the stated review policy because this affects live inference output. Evidence: diff_diff/survey.py:L538-L541, diff_diff/survey.py:L1300-L1333, diff_diff/survey.py:L1399-L1408, diff_diff/linalg.py:L1854-L1860, diff_diff/continuous_did.py:L529-L545, diff_diff/efficient_did.py:L1319-L1321. Concrete fix: have the replicate variance helpers return the valid replicate count (or effective survey d.f.), thread that through estimator state/metadata, and use n_valid - 1 everywhere safe_inference() currently consumes df_survey.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• No separate findings beyond the methodology issues above.

Tech Debt

• Severity: P3. Impact: the new TODO entry records the stale-replicate-d.f. limitation, but that deferral is not sufficient for merge because the issue changes active inference output, not just future cleanup; see TODO.md:L55-L55. Concrete fix: keep the tracking note if useful, but still implement the d.f. fix before merge.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: the new tests only pin happy-path replicate behavior. They verify raw SurveyDesign.resolve() metadata and that CS replicate fits return finite output, but they do not fail when CS loses replicate metadata during unit collapse or when dropped replicates should lower df_survey. Evidence: tests/test_survey_phase6.py:L399-L411, tests/test_survey_phase6.py:L486-L499, tests/test_survey_phase6.py:L850-L865. Concrete fix: add one CS regression that asserts result.survey_metadata.replicate_method == "JK1" and replicate-aware df_survey after fitting, plus one regression that forces invalid replicates and checks inference uses n_valid - 1.

Path to Approval

1. Make CallawaySantAnna’s unit-level survey collapse preserve replicate metadata, then route overall/event-study/group analytical inference and metadata recomputation through the replicate-aware unit design.
2. Propagate effective valid-replicate counts out of compute_replicate_vcov() / compute_replicate_if_variance() and use n_valid - 1 for all t-based inference paths that currently consume df_survey.
3. Add targeted regressions for both cases: CS replicate-weight aggregation preserving replicate metadata/dispatch, and invalid-replicate d.f. reduction.

I could not execute the test suite in this sandbox because pytest and numpy are unavailable here.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 91d778f2e44ee716ce1fc94af01a42295692bd36

---

Overall Assessment

⛔ Blocker

Executive Summary

• The prior CallawaySantAnna replicate-collapse blocker appears fixed: unit-level collapse now preserves replicate metadata and CS aggregation dispatches to replicate IF variance when uses_replicate_variance is true.
• The replicate/bootstrap rejections and the SunAbraham replicate-design rejection are documented in docs/methodology/REGISTRY.md, so I did not count those deviations as defects.
• P0: the new valid-replicate-df fix mutates ResolvedSurveyDesign.n_replicates in place inside the replicate IF helper, so repeated SE calculations on the same resolved design can silently drop still-valid replicate columns after the first invalid one.
• P1: the previous df_survey = n_valid - 1 issue is still not fully resolved. CallawaySantAnna and EfficientDiD both cache survey d.f. before replicate filtering, so p-values/CIs can still use stale R-1.
• P1 [Newly identified]: zero-weight/subpopulation support is only partially propagated. Survey-weighted CS paths still divide by zero effective weight mass instead of handling empty effective cells explicitly.
• The new tests are mostly happy-path and would not catch the invalid-replicate call-order bug, stale estimator-level d.f., or zero-mass subpopulation regressions.

Methodology

• Severity: P0. Impact: compute_replicate_if_variance() now rewrites resolved.n_replicates after dropping invalid replicates, but the same resolved design object is reused for multiple SE computations. In a supported Phase 6 subpopulation/domain workflow, an interior replicate column can become all-zero after weight-zeroing; the first call sees all R columns, then mutates n_replicates, and later calls only scan a truncated prefix of columns. That makes later SEs depend on call order and silently drops valid replicate columns, which is wrong replicate-weight inference. Evidence: diff_diff/survey.py:L1356-L1406, diff_diff/continuous_did.py:L1233-L1252, diff_diff/continuous_did.py:L640-L647, diff_diff/efficient_did.py:L1085-L1092, diff_diff/staggered_aggregation.py:L471-L482, diff_diff/survey.py:L385-L490, docs/methodology/REGISTRY.md:L2009-L2034. Concrete fix: make the replicate helpers pure. Return the valid replicate mask/count (or effective d.f.) without mutating ResolvedSurveyDesign, and reuse one filtered replicate set consistently for every SE/CI/metadata computation in a fit. Apply the same change to compute_replicate_vcov() as well.
• Severity: P1. Impact: the earlier valid-replicate d.f. finding remains open in estimator code. CallawaySantAnna snapshots precomputed["df_survey"] before any replicate filtering and reuses it in batch/aggregate inference; EfficientDiD snapshots self._survey_df in fit() and also reuses it later. So even when replicate variance is computed from fewer valid replicates, downstream p-values/CIs still use the original R-1, contrary to the new registry/TODO claim that downstream inference uses n_valid - 1. Evidence: diff_diff/staggered.py:L503-L505, diff_diff/staggered.py:L1393-L1491, diff_diff/staggered_aggregation.py:L616-L621, diff_diff/staggered_aggregation.py:L709-L714, diff_diff/efficient_did.py:L519-L524, diff_diff/efficient_did.py:L885-L917, docs/methodology/REGISTRY.md:L2032-L2034, TODO.md:L55. Concrete fix: propagate n_valid out of the variance helper, update estimator-local survey d.f. after valid-replicate filtering is finalized, and pass that fresh value into every safe_inference() / safe_inference_batch() call. The new TODO/REGISTRY text does not mitigate this under the review policy because live inference output is still wrong.

Code Quality

• Severity: P1 [Newly identified]. Impact: Phase 6 explicitly supports zero-weight subpopulations, but zero-mass handling is still partial. CallawaySantAnna continues to normalize sw_treated, sw_control, and weights_control without checking that their sums are positive, and TripleDifference still has unguarded weighted fallbacks / weighted denominators. Supported subpopulation/domain fits can therefore produce divide-by-zero, inf, or uncontrolled NaN instead of explicit empty-effective-cell handling. Evidence: diff_diff/survey.py:L385-L490, docs/methodology/REGISTRY.md:L2055-L2067, diff_diff/staggered.py:L1714-L1719, diff_diff/staggered.py:L1750-L1755, diff_diff/staggered.py:L1868-L1875, diff_diff/staggered.py:L2123-L2139, diff_diff/triple_diff.py:L1212-L1217, diff_diff/triple_diff.py:L1384-L1457. Concrete fix: before every survey-weight normalization or np.average(..., weights=...) on the new zero-weight paths, require positive effective mass; otherwise return np.nan / skip the cell or raise a clear empty-domain error. Mirror the new guard pattern already added in ContinuousDiD and Bacon.

Performance

• No findings.

Maintainability

• No separate findings beyond the stateful replicate-helper mutation above.

Tech Debt

• Severity: P3. Impact: TODO.md now marks the replicate-weight survey-d.f. item resolved even though the estimator-level propagation bug above is still live. That does not mitigate the inference issue and makes it easier to miss later. Evidence: TODO.md:L55. Concrete fix: reopen that item or add a new TODO until all estimator inference paths actually consume n_valid - 1.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: the new tests do not pin the blockers above. The helper zero-weight test uses only all-valid replicate columns, and the estimator-level replicate tests only assert finite output / metadata presence, so they would not fail if a middle replicate column became all-zero after subpopulation, if repeated SE calls shrank the active replicate set, or if CS/EfficientDiD kept using stale d.f. Evidence: tests/test_survey_phase6.py:L771-L799, tests/test_survey_phase6.py:L850-L899. Concrete fix: add targeted regressions that 1) create a subpopulation making one interior replicate column all-zero and assert repeated overall/event-study/group SEs use the same valid mask, 2) assert df_survey == n_valid - 1 in CS and EfficientDiD inference outputs, and 3) assert zero-mass subpopulation cells in CS/TripleDifference are handled explicitly.

Path to Approval

1. Refactor compute_replicate_if_variance() / compute_replicate_vcov() so they do not mutate ResolvedSurveyDesign; return the valid replicate mask/count explicitly and keep the replicate set fixed for the whole fit.
2. Thread n_valid - 1 through estimator state and inference calls, at minimum in CallawaySantAnna and EfficientDiD, instead of using cached pre-filter df_survey values.
3. Add positive-mass guards to every survey-weight normalization / weighted average still exposed by the new zero-weight/subpopulation support, at minimum the remaining CS and TripleDifference survey-weighted paths.
4. Add regression tests for invalid middle replicate columns after subpopulation, estimator-level stale-d.f. usage, and zero-effective-mass domain cells.

I could not execute the test suite in this sandbox because numpy, pandas, and pytest are unavailable here.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 84809cdc4320f7ee614d136728ece3c14100cbd4

---

Overall Assessment

⛔ Blocker

Executive Summary

• The prior stateful replicate-helper blocker appears fixed: compute_replicate_if_variance() no longer mutates ResolvedSurveyDesign.n_replicates.
• The ContinuousDiD anticipation filter fix and the EfficientDiD hausman_pretest() cluster-remap fix both look correct and are covered by targeted tests.
• The documented deviations in the registry are handled correctly: SunAbraham’s replicate-weight rejection and the replicate+bootstrap rejections in CS/ContinuousDiD/EfficientDiD are explicitly noted in the methodology registry, so I did not count them as defects.
• P0: the new _replicate_df cache in LinearRegression is never cleared, so reusing the same model instance after a replicate-weight fit can silently apply the wrong survey d.f. to a later non-replicate survey fit.
• P1: replicate-weight d.f. is still not propagated from n_valid into downstream inference/metadata for IF-based replicate paths in CallawaySantAnna, ContinuousDiD, and EfficientDiD, despite the new REGISTRY/TODO text claiming this is resolved.
• P1: CallawaySantAnna still lacks explicit zero-effective-mass guards on survey-weight normalizations, so Phase 6 subpopulation/domain fits can still hit divide-by-zero/NaN behavior instead of clean empty-domain handling.

Methodology

• Severity: P1. Impact: Affected methods: Replicate Weight Variance for CallawaySantAnna aggregation, ContinuousDiD, and EfficientDiD. The new registry/TODO text says invalid replicates are dropped and df_survey = n_valid - 1, but the estimator code still caches raw R-1 from the resolved design and discards the returned n_valid. That leaves t-based p-values/CIs and displayed survey d.f. wrong whenever subpopulation or unit-collapsed replicate designs zero out replicate columns. Evidence: docs/methodology/REGISTRY.md:L2023-L2034, TODO.md:L55-L55, diff_diff/staggered.py:L503-L505, diff_diff/staggered_aggregation.py:L476-L482, diff_diff/staggered.py:L1391-L1492, diff_diff/staggered_aggregation.py:L616-L621, diff_diff/staggered_aggregation.py:L709-L714, diff_diff/efficient_did.py:L523-L524, diff_diff/efficient_did.py:L1085-L1092, diff_diff/efficient_did.py:L885-L917, diff_diff/efficient_did.py:L1319-L1385, diff_diff/continuous_did.py:L1237-L1292, diff_diff/continuous_did.py:L529-L545, diff_diff/continuous_did.py:L657-L659. Concrete fix: propagate an effective replicate d.f. out of the replicate helpers and thread it into every safe_inference*() call and every compute_survey_metadata() call used by these estimators.

• Severity: P3. Impact: The SunAbraham replicate-weight rejection and the replicate+bootstrap rejections in CallawaySantAnna, ContinuousDiD, and EfficientDiD match explicit registry notes, so these are documented deviations rather than methodology defects. Evidence: docs/methodology/REGISTRY.md:L2026-L2034, diff_diff/sun_abraham.py:L504-L512, diff_diff/staggered.py:L1520-L1527, diff_diff/continuous_did.py:L1322-L1326, diff_diff/efficient_did.py:L948-L955. Concrete fix: none.

Code Quality

• Severity: P0. Impact: LinearRegression.fit() stores _replicate_df on replicate-weight fits but never clears it; a later non-replicate survey fit on the same model instance still overrides survey_df_ with the stale replicate d.f., and get_inference() then uses that wrong d.f. for p-values/CIs. This is silent wrong statistical output in a public API. Evidence: diff_diff/linalg.py:L1618-L1628, diff_diff/linalg.py:L1819-L1820, diff_diff/linalg.py:L1858-L1866, diff_diff/linalg.py:L2019-L2042. Concrete fix: reset self._replicate_df = None at the start of every fit() (or assign it in both branches) and only apply the override when the current fit actually uses replicate variance.

• Severity: P1. Impact: CallawaySantAnna still normalizes survey weights without checking positive effective mass in multiple survey branches. Because Phase 6 now explicitly supports zero weights for subpopulation/domain analysis, these paths can still divide by zero or produce uncontrolled NaNs instead of explicit empty-domain handling. Evidence: diff_diff/staggered.py:L800-L810, diff_diff/staggered.py:L1714-L1735, diff_diff/staggered.py:L1750-L1755, diff_diff/staggered.py:L1871-L1877, diff_diff/staggered.py:L2127-L2168. The contrast with the new explicit ContinuousDiD guard makes the gap clear: diff_diff/continuous_did.py:L878-L888. Concrete fix: before every survey-weight normalization or composite-weight normalization in CS, require positive treated/control/effective-control mass and then either skip the cell, return NaN, or raise a clear domain error consistently.

Performance

No findings.

Maintainability

No findings beyond the live inference-state and d.f.-propagation issues above.

Tech Debt

• Severity: P3. Impact: TODO.md now marks replicate-weight survey d.f. handling resolved even though the estimator code still uses raw R-1 in live inference/metadata paths. That will make future review and debugging harder because the tracking artifact now disagrees with behavior. Evidence: TODO.md:L55-L55, docs/methodology/REGISTRY.md:L2032-L2034. Concrete fix: reopen or remove the resolved entry until the code actually updates estimator d.f. to n_valid - 1.

Security

No findings.

Documentation/Tests

• Severity: P2. Impact: the new tests mostly lock in happy-path R-1 metadata and finite outputs; they do not exercise dropped/all-zero replicate columns after subpopulation or refitting the same LinearRegression instance across survey designs, so the live bugs above would pass. Evidence: tests/test_survey_phase6.py:L496-L499, tests/test_survey_phase6.py:L867-L882, tests/test_survey_phase6.py:L884-L899. Concrete fix: add regressions for 1) a subpopulation that makes an interior replicate column all-zero and assert df_survey == n_valid - 1 in CS/ContinuousDiD/EfficientDiD metadata and inference, 2) reusing a LinearRegression instance across replicate then non-replicate survey fits, and 3) CS zero-effective-mass domain cells.

Path to Approval

1. Reset _replicate_df at the start of every LinearRegression.fit() and only override survey_df_ on the current replicate fit; add a regression test that reuses one LinearRegression instance across replicate and non-replicate survey designs.
2. Propagate effective valid-replicate d.f. into result metadata and every safe_inference*() call in CallawaySantAnna, ContinuousDiD, and EfficientDiD so dropped replicate columns produce df_survey = n_valid - 1.
3. Add explicit positive-mass guards to every survey-weight normalization path in CallawaySantAnna and choose a consistent empty-domain behavior (skip cell, NaN, or clear ValueError).
4. After the code matches behavior, keep the REGISTRY/TODO resolved language; otherwise reopen or downgrade those entries.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 5434007cf9a0c5fc976bbf0cb88720c6d7d4a6e4

---

Overall Assessment

⚠️ Needs changes

Re-review result: the prior CallawaySantAnna zero-effective-mass blocker appears fixed, and the ContinuousDiD anticipation / EfficientDiD hausman_pretest() fixes look correct. One prior P1 remains in the new replicate-weight methodology work. This review is based on static diff inspection.

Executive Summary

• The previous CallawaySantAnna zero-mass bug looks resolved in both the general and vectorized ATT paths.
• ContinuousDiD’s event-study anticipation filter fix and EfficientDiD’s clustered hausman_pretest() remap both match the intended fixes.
• P1 remains: IF-based replicate-weight inference still uses raw R - 1 survey d.f. after subpopulation/domain restrictions can invalidate replicate columns; CallawaySantAnna only partially propagates the effective replicate count.
• The SunAbraham replicate rejection and the replicate+bootstrap rejections in CallawaySantAnna, ContinuousDiD, and EfficientDiD are explicitly documented in REGISTRY.md, so they are P3 informational rather than defects.
• The new Phase 6 tests still do not cover the estimator-level “subpopulation zeros an entire replicate column” case, so the remaining P1 is not locked down.

Methodology

• Severity: P1. Impact: IF-based replicate-weight paths still overstate survey d.f. after supported subpopulation/domain restrictions can zero whole replicate columns. SurveyDesign.subpopulation() explicitly zeros excluded replicate weights, and compute_replicate_if_variance() already returns n_valid, but ContinuousDiD, EfficientDiD, and TripleDifference discard that effective replicate count and continue to infer with raw R - 1; CallawaySantAnna only partially updates precomputed["df_survey"], leaving overall ATT inference and returned survey_metadata.df_survey stale. That makes t-based p-values/CIs too optimistic whenever n_valid < R. The new REGISTRY note saying IF-based replicate paths “essentially never fail” is not true once Phase 6 subpopulation support is used. Evidence: diff_diff/survey.py:486, diff_diff/survey.py:1399, diff_diff/staggered_aggregation.py:479, diff_diff/staggered.py:1402, diff_diff/staggered.py:1496, diff_diff/continuous_did.py:1241, diff_diff/continuous_did.py:1285, diff_diff/efficient_did.py:524, diff_diff/efficient_did.py:1088, diff_diff/triple_diff.py:581, diff_diff/triple_diff.py:1099, docs/methodology/REGISTRY.md:2032. Concrete fix: propagate n_valid - 1 from every IF-based replicate SE computation into estimator state/result metadata and all downstream safe_inference*() calls; for CallawaySantAnna, return effective d.f. alongside aggregated SEs or reread precomputed["df_survey"] after aggregation before building overall inference and metadata.

• Severity: P3. Impact: The SunAbraham replicate rejection and the replicate+bootstrap rejections in CallawaySantAnna, ContinuousDiD, and EfficientDiD are explicitly documented methodology notes, so they are acceptable documented deviations, not defects. Evidence: docs/methodology/REGISTRY.md:2026, diff_diff/sun_abraham.py:508, diff_diff/staggered.py:1529, diff_diff/continuous_did.py:1322, diff_diff/efficient_did.py:948. Concrete fix: none.

Code Quality

No unmitigated findings.

Performance

No findings.

Maintainability

No findings beyond the live replicate-d.f. issue above.

Tech Debt

• Severity: P3. Impact: TODO.md now marks the replicate-weight survey-d.f. issue as fully resolved, but the actual fix is only complete for the OLS replicate path in LinearRegression.fit(). Future reviewers will be told the issue is closed while the IF-based estimators still use raw R - 1. Evidence: TODO.md:55, diff_diff/linalg.py:1808, diff_diff/efficient_did.py:1088. Concrete fix: reopen that TODO item, or narrow it to the OLS LinearRegression.fit() path until the IF-based estimators also propagate n_valid - 1.

Security

No findings.

Documentation/Tests

• Severity: P2. Impact: The new Phase 6 tests cover replicate happy paths, subpopulation metadata preservation, and zero-weight divide-by-zero behavior, but they do not combine subpopulation with replicate inference to assert df_survey == n_valid - 1 after a replicate column becomes all-zero. That gap is why the remaining P1 still passes. Evidence: tests/test_survey_phase6.py:488, tests/test_survey_phase6.py:772, tests/test_survey_phase6.py:850, tests/test_survey_phase6.py:884. Concrete fix: add estimator-level regressions where SurveyDesign.subpopulation() zeros an entire replicate column and assert that both survey_metadata.df_survey and the corresponding t-based inference use n_valid - 1 for CallawaySantAnna overall ATT, ContinuousDiD, EfficientDiD, and TripleDifference.

Path to Approval

1. Propagate effective replicate d.f. (n_valid - 1) through all IF-based replicate branches and use it consistently in result metadata plus every safe_inference*() call for CallawaySantAnna overall aggregation, ContinuousDiD, EfficientDiD, and TripleDifference.
2. In CallawaySantAnna specifically, stop capturing df_survey before _aggregate_*() mutates precomputed["df_survey"]; either return effective d.f. from the aggregation helpers or reread the updated value before constructing overall ATT inference and survey_metadata.
3. Add regression tests that create a supported subpopulation/domain where one replicate column becomes all-zero, and assert the affected estimators report df_survey == n_valid - 1.
4. Update REGISTRY.md and TODO.md to match the actual behavior once the code is fixed.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 59302869f282fd962e71be91c14b7c752a7540fd

---

Overall Assessment

⚠️ Needs changes

The prior zero-effective-mass fix in CallawaySantAnna, the ContinuousDiD anticipation filter fix, and the EfficientDiD hausman_pretest() cluster-count fix all look correct on the current diff. The remaining blocker is narrower than in the last review, but it is still live: effective replicate survey d.f. is not propagated through all IF-based replicate-weight paths.

Executive Summary

• The previous CallawaySantAnna zero-mass issue appears fixed, and the ContinuousDiD anticipation/event-study change matches the registry intent.
• EfficientDiD’s hausman_pretest() now correctly recomputes the effective cluster count after dropping non-finite EIF rows.
• P1 remains: EfficientDiD and TripleDifference still use raw replicate d.f. (R - 1) for t-based inference after subpopulation/domain restrictions can invalidate replicate columns.
• ContinuousDiD now propagates effective replicate d.f. into analytical inference, and CallawaySantAnna now rereads updated d.f. for overall ATT, but both still leave stale survey_metadata.df_survey in results.
• The SunAbraham replicate rejection and replicate+bootstrap rejections are documented in REGISTRY.md, so those are P3 informational, not defects.
• The new Phase 6 tests cover replicate happy paths and bootstrap rejections, but they still do not exercise the “subpopulation zeros an entire replicate column” case that would catch the remaining d.f. bug.

Methodology

• Severity: P1. Impact: Replicate Weight Variance + Subpopulation Analysis are still incorrectly wired in EfficientDiD and TripleDifference. SurveyDesign.subpopulation() explicitly zeros excluded replicate columns, so a supported domain restriction can make one or more replicate columns invalid (diff_diff/survey.py:L385-L505). compute_replicate_if_variance() already returns n_valid for exactly this reason (diff_diff/survey.py:L1353-L1424). EfficientDiD still sets _survey_df once from raw R - 1 (diff_diff/efficient_did.py:L519-L524) and then discards _n_valid when computing replicate-weight SEs (diff_diff/efficient_did.py:L1078-L1092); all of its safe_inference() calls continue to use that stale df (diff_diff/efficient_did.py:L885-L887, diff_diff/efficient_did.py:L915-L917, diff_diff/efficient_did.py:L1319-L1321, diff_diff/efficient_did.py:L1383-L1385). TripleDifference has the same problem: it ignores _nv from compute_replicate_if_variance() (diff_diff/triple_diff.py:L1088-L1100) and still computes inference from the original survey_metadata.df_survey produced at resolve-time (diff_diff/triple_diff.py:L489-L490, diff_diff/triple_diff.py:L578-L588). That makes t-based p-values/CIs too optimistic whenever n_valid < R. Concrete fix: propagate n_valid - 1 through these IF-based replicate branches, use it in every safe_inference() call, and overwrite the result metadata with the same effective df.
• Severity: P3. Impact: The SunAbraham replicate rejection and the replicate+bootstrap rejections are documented deviations, not defects. The current code matches the explicit Notes in the Methodology Registry for SunAbraham and for replicate-weight + bootstrap rejection in CallawaySantAnna, ContinuousDiD, and EfficientDiD (docs/methodology/REGISTRY.md:L2026-L2031, diff_diff/sun_abraham.py:L504-L514, diff_diff/staggered.py:L1531-L1543, diff_diff/continuous_did.py:L1329-L1336, diff_diff/efficient_did.py:L947-L959). Concrete fix: none.

Code Quality

• No unmitigated findings.

Performance

• No findings.

Maintainability

• Severity: P2. Impact: ContinuousDiD and CallawaySantAnna still let survey_metadata.df_survey drift away from the df actually used in inference. ContinuousDiD now computes a corrected analytical _survey_df (diff_diff/continuous_did.py:L517-L530, diff_diff/continuous_did.py:L1287-L1300), but then rebuilds survey_metadata from the unadjusted unit-level replicate design (diff_diff/continuous_did.py:L523-L527) and returns it unchanged (diff_diff/continuous_did.py:L724-L725). CallawaySantAnna now rereads updated precomputed["df_survey"] for overall ATT inference (diff_diff/staggered.py:L1496-L1502), but its stored survey_metadata was computed earlier and never updated after replicate aggregation mutates the effective df (diff_diff/staggered.py:L1392-L1402, diff_diff/staggered_aggregation.py:L476-L482, diff_diff/staggered.py:L1650-L1670). That leaves summary()/to_dict() reporting raw R - 1 even when inference used a smaller effective df. Concrete fix: after replicate aggregation, overwrite survey_metadata.df_survey (and displayed replicate count if relevant) with the effective n_valid - 1 before constructing results.

Tech Debt

• Severity: P3. Impact: TODO.md now says the replicate-weight survey-d.f. issue is resolved, but that is not true for the live IF-based estimator paths. The new entry marks the item resolved (TODO.md:L55-L55), while EfficientDiD and TripleDifference still behave as above. Concrete fix: reopen or narrow the TODO entry until the IF-based paths also propagate n_valid - 1.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: The new tests still do not exercise the supported subpopulation case that invalidates whole replicate columns. The added coverage checks replicate happy paths and metadata preservation (tests/test_survey_phase6.py:L850-L958), plus raw subpopulation zeroing (tests/test_survey_phase6.py:L884-L899), but it never creates a domain restriction that zeros an entire replicate column and then asserts corrected df_survey or t-based inference in EfficientDiD, TripleDifference, ContinuousDiD, or CallawaySantAnna. That gap is why the remaining d.f. bug still passes. Concrete fix: add estimator-level regressions where SurveyDesign.subpopulation() wipes out a full replicate column and assert both survey_metadata.df_survey and the resulting p-values/CIs use n_valid - 1.
• Severity: P3. Impact: The new REGISTRY note overstates the current implementation. It says IF-based replicate paths “essentially never fail” and therefore do not need df propagation (docs/methodology/REGISTRY.md:L2032-L2036), but the new subpopulation API makes n_valid < R a supported and testable case (diff_diff/survey.py:L385-L505, diff_diff/survey.py:L1399-L1408). Concrete fix: revise the note once the code is fixed, or scope it narrowly to paths that truly cannot lose replicates.

Path to Approval

1. In EfficientDiD, thread effective replicate count through _compute_survey_eif_se() and all aggregation paths, update self._survey_df to n_valid - 1, and use that same df in result metadata.
2. In TripleDifference, replace the current resolve-time survey df with the _nv returned by compute_replicate_if_variance(), and use n_valid - 1 for both safe_inference() and survey_metadata.df_survey.
3. In ContinuousDiD and CallawaySantAnna, keep the inference behavior that is now mostly correct, but also overwrite survey_metadata.df_survey so result objects match the statistics actually reported.
4. Add regression tests where SurveyDesign.subpopulation() zeros an entire replicate column and assert df_survey == n_valid - 1 plus matching t-based p-values/CIs for EfficientDiD, TripleDifference, ContinuousDiD, and CallawaySantAnna.
5. Reopen or narrow TODO.md’s resolved entry and revise the REGISTRY note after the IF-based replicate d.f. propagation is actually complete.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 86f09f2de8c3d888024bf75ba8777b1cd861b8fa

---

Overall Assessment

⚠️ Needs changes

The prior EfficientDiD hausman_pretest() cluster fix and the ContinuousDiD anticipation/event-study fix both look correct on the current diff. The remaining blocker is narrower: TripleDifference now carries replicate d.f. state across fits, so reusing the same estimator instance can silently apply the wrong survey d.f. to a later fit.

Executive Summary

• The earlier P1s around EfficientDiD’s stale n_cl and ContinuousDiD’s missing anticipation filter appear resolved in the changed code.
• Single-fit replicate-d.f. propagation is materially improved in EfficientDiD and TripleDifference.
• P1 remains: TripleDifference never resets _replicate_n_valid, so a prior replicate-weight fit can contaminate a later non-replicate survey fit on the same object.
• ContinuousDiD, EfficientDiD, and CallawaySantAnna still let user-visible survey metadata drift from the effective replicate count used in inference; this is now more visible because summaries print Replicates: and Survey d.f. explicitly.
• The new TODO/REGISTRY text overstates the replicate-d.f. fix: n_replicates is not actually updated after invalid replicates are dropped.
• The new Phase 6 tests cover happy-path replicate fits and rejection paths, but they still miss repeated-fit state contamination and effective-metadata assertions.

Methodology

• Severity: P1. Impact: TripleDifference can report incorrect p-values/CIs on a later survey fit because _replicate_n_valid is only ever set inside the replicate branch, never cleared at the start of fit(), and then unconditionally overrides the current survey d.f. whenever the attribute exists. In the current code, _replicate_n_valid is read in diff_diff/triple_diff.py:L580-L585, but it is only assigned in the replicate path at diff_diff/triple_diff.py:L1092-L1106; there is no reset in the new fit() setup at diff_diff/triple_diff.py:L481-L490. A replicate-weight fit followed by a weights-only or strata/PSU survey fit on the same estimator instance can therefore silently reuse the old reduced d.f. Concrete fix: reset self._replicate_n_valid = None at the top of fit() and/or gate the override on resolved_survey.uses_replicate_variance for the current fit only.

• Severity: P3. Impact: The new bootstrap rejections and SunAbraham replicate rejection match documented registry notes, so they are not defects. See docs/methodology/REGISTRY.md:L2026-L2036, diff_diff/sun_abraham.py:L499-L510, diff_diff/staggered.py:L1531-L1543, diff_diff/continuous_did.py:L1329-L1336, and diff_diff/efficient_did.py:L947-L959. Concrete fix: none.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• Severity: P2. Impact: Result metadata still reports raw replicate counts / raw R - 1 even when inference has already switched to an effective n_valid - 1. ContinuousDiD computes a corrected analytical d.f. but then rebuilds metadata from the unmodified unit-level resolved design at diff_diff/continuous_did.py:L517-L527 after deriving the effective d.f. at diff_diff/continuous_did.py:L1287-L1300. EfficientDiD does the same via _recompute_unit_survey_metadata() at diff_diff/efficient_did.py:L1056-L1073 even though _survey_df may have been reduced at diff_diff/efficient_did.py:L1092-L1095. CallawaySantAnna updates precomputed["df_survey"] during replicate aggregation at diff_diff/staggered_aggregation.py:L476-L482, but the survey_metadata passed into results is still the earlier object from diff_diff/staggered.py:L1392-L1398 and diff_diff/staggered.py:L1650-L1670. Because summaries now print Replicates: and Survey d.f. directly at diff_diff/results.py:L14-L45, the mismatch is user-visible. Concrete fix: when n_valid < R, overwrite survey_metadata.n_replicates/df_survey from the effective count before constructing results, or drop invalid replicate columns from the resolved design so metadata stays authoritative.

Tech Debt

• Severity: P3. Impact: The new TODO entry says the replicate-d.f. issue is resolved because n_replicates is updated after invalid replicates are dropped, but that is not what the implementation does. The claim appears in TODO.md:L55-L55, while ResolvedSurveyDesign.subset_to_units() preserves the original n_replicates at diff_diff/survey.py:L552-L592 and compute_survey_metadata() copies that raw count into results at diff_diff/survey.py:L638-L690. Concrete fix: narrow or reopen the TODO entry until the effective count is actually persisted into result metadata.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: The new Phase 6 tests validate one-off replicate fits and rejection paths, but they do not cover the repeated-fit contamination bug or the still-stale metadata. The new estimator tests at tests/test_survey_phase6.py:L850-L957 always instantiate fresh estimators, and there is no regression asserting corrected survey_metadata.df_survey/n_replicates after invalid replicate columns are dropped. The registry also overstates the current implementation by claiming IF-based paths do not need d.f. propagation at docs/methodology/REGISTRY.md:L2032-L2036. Concrete fix: add 1) a repeated-fit regression for TripleDifference and 2) estimator-level metadata assertions for effective replicate count/d.f. after subpopulation invalidates replicate columns; then revise the registry note to match the implemented behavior.

Path to Approval

1. Reset or scope _replicate_n_valid in TripleDifference.fit() so only the current fit’s replicate path can override survey d.f.
2. Add a regression test that reuses the same TripleDifference instance for two fits: first with replicate weights, then with a non-replicate survey design, and assert the second fit’s df_survey, p_value, and CI use the current design rather than the stale replicate count.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 978e8662e9c98a137b93a17ee149b808db80b00b

---

Overall Assessment

⚠️ Needs changes

The earlier TripleDifference stale-state fix and the EfficientDiD.hausman_pretest() cluster recount fix look correct on the current diff, but the new replicate-weight feature still hardcodes a narrower survey-design contract than it claims, and that can yield incorrect replicate-based SEs for otherwise valid inputs.

Executive Summary

• The prior P1 around stale replicate-d.f. state in TripleDifference appears fixed by the reset at diff_diff/triple_diff.py:L481-L482.
• The prior EfficientDiD Hausman issue also appears fixed: n_cl is now recomputed after NaN-row filtering at diff_diff/efficient_did.py:L1596-L1601.
• The remaining blocker is the new replicate-weight API: it silently assumes one combined-weight/default-scale/no-FPC convention, but the code never represents or validates that assumption.
• In the IF-based replicate path, each replicate column is renormalized before w_r / w_full is formed, so SEs can change purely because replicate columns have different totals.
• Result metadata, TODO.md, and the registry still overstate the replicate-d.f. fix: inference may use an effective n_valid - 1, but summaries and docs still report raw R - 1.
• The new tests cover canonical happy-path replicate designs and rejection paths, but not combined.weights=False, nondefault scaling, replicate FPC, unequal-total replicate columns, or effective-metadata assertions.

Methodology

• Severity: P1. Affected methods: Replicate Weight Variance (BRR, Fay, JK1, JKn) across SurveyDesign, LinearRegression, CallawaySantAnna aggregated SEs, ContinuousDiD, EfficientDiD, and TripleDifference. Impact: the implementation assumes a single replicate-design convention but neither models nor validates it. SurveyDesign has no fields for combined_weights, scale, rscales, or jackknife replicate fpc/fpctype diff_diff/survey.py:L58-L68, diff_diff/survey.py:L107-L117, compute_replicate_vcov() treats replicate columns as full weights directly diff_diff/survey.py:L1280-L1333, and the IF path first renormalizes each replicate column diff_diff/survey.py:L202-L208 and then forms w_r / w_full diff_diff/survey.py:L1386-L1408. That ratio step is not invariant to replicate-specific rescaling, so IF-based SEs can move when replicate totals differ, affecting the new survey aggregation paths in diff_diff/staggered_aggregation.py:L476-L485, diff_diff/continuous_did.py:L1233-L1247, diff_diff/efficient_did.py:L1085-L1095, and diff_diff/triple_diff.py:L1095-L1109. Standard replicate-design interfaces expose combined.weights, scale, rscales, and JK1/JKn fpc, and they permit BRR inputs with combined.weights=FALSE. Concrete fix: add those design parameters and preserve raw replicate-to-full ratios in the IF path, or explicitly restrict support to the exact canonical subset and reject everything else before variance is computed. (r-survey.r-forge.r-project.org)
• Severity: P3. Impact: the new SunAbraham replicate rejection and the replicate+bootstrap rejections in CallawaySantAnna, ContinuousDiD, and EfficientDiD match documented registry notes, so they are not defects. See docs/methodology/REGISTRY.md:L2026-L2031, diff_diff/sun_abraham.py:L499-L514, diff_diff/staggered.py:L1531-L1543, diff_diff/continuous_did.py:L1329-L1336, and diff_diff/efficient_did.py:L947-L959. Concrete fix: none.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• Severity: P2. Impact: effective replicate d.f. still does not flow into user-visible survey metadata. ContinuousDiD and EfficientDiD compute a corrected analytical df separately diff_diff/continuous_did.py:L517-L530, diff_diff/efficient_did.py:L1085-L1095, but then rebuild survey_metadata from raw ResolvedSurveyDesign.n_replicates / df_survey diff_diff/continuous_did.py:L520-L527, diff_diff/efficient_did.py:L1056-L1073. CallawaySantAnna updates only precomputed["df_survey"] during aggregation diff_diff/staggered_aggregation.py:L476-L482, but the final results still carry the earlier raw metadata diff_diff/staggered.py:L1392-L1398, diff_diff/staggered.py:L1650-L1670. Because summaries now print Replicates: and Survey d.f. directly diff_diff/results.py:L32-L44, users can see metadata that disagrees with the inference actually used. Concrete fix: persist the effective replicate count/d.f. into SurveyMetadata before results construction and assert it in results-level tests.

Tech Debt

• Severity: P3. Impact: TODO.md says the replicate-d.f. issue is resolved because n_replicates is updated after invalid replicates are dropped TODO.md:L55-L55, and the registry still says IF-based replicate paths do not need d.f. propagation docs/methodology/REGISTRY.md:L2032-L2036, but neither statement matches the shipped code. Concrete fix: narrow or reopen the TODO entry and update the registry note to describe the current behavior exactly.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: the new Phase 6 tests cover canonical combined-weight toy replicates and the rejection paths tests/test_survey_phase6.py:L399-L677, tests/test_survey_phase6.py:L850-L1022, but they do not cover the methodology-sensitive cases that would catch the blocker: combined.weights=False, nondefault scale/rscales, JK replicate FPC, unequal-total replicate columns in the IF-based path, or corrected metadata after dropped replicates. The roadmap also now states that “All phases (1-6) are implemented” docs/survey-roadmap.md:L3-L4, which overstates the current replicate-design support. Concrete fix: add those regression tests and either narrow the roadmap/registry wording to the actually supported subset or implement the missing design controls.

Path to Approval

1. Extend SurveyDesign / ResolvedSurveyDesign in diff_diff/survey.py to carry combined_weights, scale, rscales, and jackknife replicate fpc/fpctype, and thread them through compute_replicate_vcov() and compute_replicate_if_variance().
2. Remove the per-column replicate renormalization from the IF-based path, or otherwise ensure compute_replicate_if_variance() uses the true raw replicate-to-full ratios instead of ratios after replicate-specific rescaling.
3. Add regression tests for: a BRR design with combined.weights=False; a JK1/JKn design needing explicit scale or FPC; an IF-based replicate case with unequal replicate-column totals; and a results-metadata assertion that Replicates: / Survey d.f. match the effective n_valid - 1 path when replicates are dropped.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 088bdc2f62fa591e3b1199663753090c653b63f2

---

Overall Assessment

⚠️ Needs changes

The prior hausman_pretest() cluster recount fix looks correct in diff_diff/efficient_did.py:L1596, the ContinuousDiD anticipation filter is now applied in the event-study aggregation at diff_diff/continuous_did.py:L1075, and replicate columns are no longer renormalized on resolve at diff_diff/survey.py:L217. The remaining blocker is that the new replicate-design parameters are not propagated through the helper constructors that the estimator paths actually use.

Static review only; I could not execute the new tests in this environment because the available Python runtime is missing the project deps.

Executive Summary

• The earlier EfficientDiD.hausman_pretest() stale-n_cl issue appears fixed at diff_diff/efficient_did.py:L1596.
• The ContinuousDiD event-study anticipation bug also appears fixed at diff_diff/continuous_did.py:L1075.
• The replicate-weight implementation now exposes the right public knobs and preserves raw replicate columns on resolve, matching the intended registry contract at diff_diff/survey.py:L65 and docs/methodology/REGISTRY.md:L2017.
• The blocker is downstream propagation: combined_weights, replicate_scale, replicate_rscales, and mse are dropped by subpopulation() and the unit-collapse helpers, so nondefault replicate designs silently revert to defaults in CS, ContinuousDiD, and EfficientDiD.
• User-visible replicate metadata is still stale when valid replicates are dropped, and TODO.md now overstates that this was fully resolved.
• The new tests cover default JK1 paths, but they do not exercise nondefault replicate settings through subpopulation() or the estimator unit-collapse paths.

Methodology

• Severity: P1. Impact: Affected methods are Replicate Weight Variance and Subpopulation Analysis. The registry now documents support for combined_weights, replicate_scale, replicate_rscales, and mse in Phase 6 at docs/methodology/REGISTRY.md:L2017, and SurveyDesign exposes those fields at diff_diff/survey.py:L65. But SurveyDesign.subpopulation() rebuilds a new design without any of them at diff_diff/survey.py:L511, ResolvedSurveyDesign.subset_to_units() drops them when collapsing to unit level at diff_diff/survey.py:L574, and CallawaySantAnna._collapse_survey_to_unit_level() drops them too at diff_diff/staggered.py:L332. Those copied designs are exactly what drive CS aggregated SEs, ContinuousDiD, and EfficientDiD at diff_diff/staggered_aggregation.py:L476, diff_diff/continuous_did.py:L1226, and diff_diff/efficient_did.py:L519. So a user who sets combined_weights=False, custom scales, or mse=False will silently get SEs computed under the default replicate formula instead. Concrete fix: thread all four fields through subpopulation(), subset_to_units(), and _collapse_survey_to_unit_level(), then add end-to-end estimator tests for each nondefault mode.
• Severity: P3. Impact: The new SunAbraham replicate rejection and the replicate+bootstrap rejections in CS, ContinuousDiD, and EfficientDiD match documented registry notes, so I am not counting them as defects. See docs/methodology/REGISTRY.md:L2032, docs/methodology/REGISTRY.md:L2035, diff_diff/sun_abraham.py:L499, diff_diff/staggered.py:L1526, diff_diff/continuous_did.py:L1329, and diff_diff/efficient_did.py:L943. Concrete fix: none.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• Severity: P2. Impact: Effective replicate d.f. still does not propagate into user-visible survey metadata. The inference paths do update d.f. when n_valid_rep < R at diff_diff/staggered_aggregation.py:L479 and diff_diff/efficient_did.py:L1091, but ContinuousDiD, EfficientDiD, and CS rebuild survey_metadata from the original resolved design at diff_diff/continuous_did.py:L520, diff_diff/efficient_did.py:L1056, and diff_diff/staggered.py:L1389. The shared summary block then prints that stale Replicates: / Survey d.f. metadata at diff_diff/results.py:L32. Concrete fix: when invalid replicates are dropped, either update ResolvedSurveyDesign.n_replicates before recomputing metadata or overwrite survey_metadata.n_replicates / df_survey explicitly before result construction.

Tech Debt

• Severity: P3. Impact: TODO.md now says the replicate-d.f. issue is resolved because n_replicates itself is updated after invalid replicates are dropped at TODO.md:L55, but I do not see any code that mutates ResolvedSurveyDesign.n_replicates; the current implementation only patches parallel df fields. That makes the TODO entry misleading for future reviewers. Concrete fix: reopen or narrow the TODO entry until metadata propagation is actually complete.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: The new tests only assert constructor-level storage of combined_weights / replicate_scale at tests/test_survey_phase6.py:L817 and tests/test_survey_phase6.py:L825, while the estimator-level replicate tests use only default JK1 settings at tests/test_survey_phase6.py:L921, tests/test_survey_phase6.py:L938, and tests/test_survey_phase6.py:L955. That gap missed the broken propagation path even though the registry now claims those modes are supported and preserved at docs/methodology/REGISTRY.md:L2017 and docs/methodology/REGISTRY.md:L2073. Concrete fix: add regression tests that run subpopulation() plus CS/ContinuousDiD/EfficientDiD with combined_weights=False, custom replicate_scale / replicate_rscales, and mse=False, and assert the copied design retains those values.

Path to Approval

1. Propagate combined_weights, replicate_scale, replicate_rscales, and mse through diff_diff/survey.py:L511, diff_diff/survey.py:L600, and diff_diff/staggered.py:L396.
2. Add end-to-end regression tests that fit CS, ContinuousDiD, and EfficientDiD under nondefault replicate settings, including a subpopulation() case on a replicate design.
3. When n_valid_rep < R, propagate the effective replicate count into survey_metadata before result construction and update the TODO.md “resolved” entry so the printed summary and tracked status match the implemented behavior.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: aabb4044710882acf4ccffaaed0d0efe71d97d1a

---

Overall Assessment

⚠️ Needs changes

The prior P1 propagation issue looks fixed: replicate metadata now survives subpopulation() and the unit-collapse helpers, and the ContinuousDiD anticipation / EfficientDiD Hausman fixes look correct. The remaining blocker is in replicate-weight variance: when some replicates are dropped, the new code recomputes the BRR/JK scaling from n_valid instead of preserving the design’s original scale. Static review only; I could not run the tests here because the sandbox Python environment is missing numpy.

Executive Summary

• The earlier replicate-parameter propagation bug appears resolved in diff_diff/survey.py:L510-L526, diff_diff/survey.py:L578-L621, and diff_diff/staggered.py:L382-L413.
• The ContinuousDiD anticipation fix looks correct in diff_diff/continuous_did.py:L1075-L1078, and the EfficientDiD stale-n_cl fix looks correct in diff_diff/efficient_did.py:L1592-L1601.
• P1: diff_diff/survey.py:L1333-L1398 and diff_diff/survey.py:L1453-L1490 rescale BRR/Fay/JK/JKn variance by the number of surviving replicates, which changes SEs whenever a replicate solve fails or a subpopulation makes a replicate empty.
• P2: the PR explicitly allows and tests custom replicate_scale for replicate_method="BRR" in diff_diff/survey.py:L134-L144 and tests/test_survey_phase6.py:L825-L840, which yields nonstandard BRR/Fay variance while results still label the method as BRR/Fay.
• TODO.md overstates the replicate-df issue as resolved: I do not see any code updating ResolvedSurveyDesign.n_replicates, so summary metadata can still report the original replicate count even when inference patches the df elsewhere.
• The new tests cover default JK1 paths, but they do not exercise the new n_valid < R edge case introduced by dropped/empty replicates.

Methodology

• Severity: P1. Impact: Affected methods are Replicate Weight Variance and Subpopulation Analysis. In diff_diff/survey.py:L1381-L1398 and diff_diff/survey.py:L1474-L1490, the BRR/Fay/JK1/JKn multipliers are recomputed from n_valid or the count of valid replicates within each replicate stratum after invalid replicates are dropped. But the registry formulas are defined in terms of the original design R / n_h in docs/methodology/REGISTRY.md:L2003-L2042, and the survey package’s svrVar() keeps the design scale fixed when NA replicates are removed, subsetting only the valid replicate rows / rscales. Because diff_diff/survey.py:L504-L508 now zeros replicate columns outside a subpopulation, a domain restricted to a deleted PSU can make n_valid < R in exactly the new code path this PR adds. Concrete fix: preserve the original design factor when dropping invalid replicates; for fixed-scale methods, do not recompute the multiplier from n_valid, and for JKn preserve the original per-stratum replicate counts rather than the counts of surviving replicates. Add an explicit warning in the IF path when any replicate is discarded. citeturn0view0turn4view0
• Severity: P2. Impact: Standard BRR/Fay formulas are fixed-scale in docs/methodology/REGISTRY.md:L2004-L2008, but the PR now accepts and applies replicate_scale / replicate_rscales for BRR/Fay in diff_diff/survey.py:L1368-L1377 and diff_diff/survey.py:L1465-L1470, and it adds a BRR custom-scale test in tests/test_survey_phase6.py:L825-L840. If a user supplies replicate_scale under replicate_method="BRR" or "Fay", the library will still label the design as BRR/Fay even though the variance is no longer the standard BRR/Fay estimator. Concrete fix: either reject/ignore custom scale overrides for fixed-scale BRR/Fay, or document this as an explicit deviation and relabel it as a user-specified replicate design rather than standard BRR/Fay. citeturn0view0

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• Severity: P3. Impact: TODO.md:L55-L55 says the replicate-df issue is resolved because n_replicates is updated after invalid replicates are dropped, but the current code still derives metadata from the original n_replicates in diff_diff/survey.py:L563-L567 and diff_diff/survey.py:L705-L719. The new inference paths only patch temporary df fields in diff_diff/linalg.py:L1808-L1823, diff_diff/linalg.py:L1865-L1869, diff_diff/staggered_aggregation.py:L476-L482, diff_diff/continuous_did.py:L1289-L1292, and diff_diff/efficient_did.py:L1091-L1094. Concrete fix: reopen or narrow the TODO entry until result metadata is rebuilt from the effective replicate count, not just the df used in inference.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: The new tests exercise default JK1 end-to-end in tests/test_survey_phase6.py:L501-L520 and tests/test_survey_phase6.py:L930-L970, and they exercise constructor-level flags in tests/test_survey_phase6.py:L820-L840. But they do not cover the n_valid < R regime behind the remaining variance bug: there is no OLS replicate fit with one failed/singular replicate, and no estimator-level replicate-weight subpopulation() case where one replicate becomes all-zero. Concrete fix: add regression tests for both dropped-replicate paths, and if nondefault replicate settings remain supported, add at least one estimator-level fit with combined_weights=False or explicit rscales.

Path to Approval

1. In diff_diff/survey.py:L1333-L1398 and diff_diff/survey.py:L1453-L1490, keep the original replicate design scaling when invalid replicates are dropped. For BRR/Fay/JK1, do not recompute the multiplier from n_valid; for JKn, use the original per-stratum replicate counts. Add a warning when the IF path discards any replicate.
2. Add targeted regression tests for n_valid < R: one OLS replicate design with a singular/degenerate replicate solve, and one replicate-weight subpopulation() case where a replicate becomes all-zero but others remain valid.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 0bb59538ff1b634ecea57ec91c064e2a7dae34b9

---

Overall Assessment

⛔ Blocker

Static review only; I could not execute the added tests in this sandbox because the Python environment is missing numpy.

Executive Summary

• The prior propagation issue looks fixed: replicate metadata now survives subpopulation() and the unit-collapse helpers, and the ContinuousDiD anticipation / EfficientDiD stale-n_cl fixes look correct.
• Severity P0 [Newly identified]: the default IF-based replicate path divides raw combined replicate weights by normalized full-sample weights, so SEs, p-values, and CIs are silently wrong whenever the original weights do not already sum to n.
• Severity P1: the earlier dropped-replicate scaling issue is only partially resolved; the JKn branches still recompute per-stratum factors from surviving replicates instead of the original n_h.
• The new DEFF diagnostics and the zero-weight guards look reasonable from a methodology standpoint.
• The new tests are broad, but they miss both remaining variance bugs because the IF tests either use unit-sum weights or construct ResolvedSurveyDesign directly instead of exercising SurveyDesign.resolve().

Methodology

• Severity: P0 [Newly identified]. Impact: affected method is Replicate Weight Variance, specifically the IF-based combined_weights=True path used by CallawaySantAnna aggregation, ContinuousDiD, EfficientDiD, and TripleDifference. The registry says replicate contrasts must use w_r / w_full, but SurveyDesign.resolve() normalizes full-sample pweight/aweight values to sum n while leaving replicate columns raw, and compute_replicate_if_variance() then uses those normalized full weights as the denominator. If raw full weights sum to W, the implemented ratio becomes w_r / ((n/W) w_full) = (W/n) * (w_r / w_full) rather than the documented ratio, so replicate contrasts are systematically rescaled unless W = n. That is silent wrong statistical output for the default analytical replicate path. Concrete fix: preserve raw full-sample weights on replicate designs (or skip full-weight normalization in the replicate-design branch) and use the raw denominator in compute_replicate_if_variance(); add a regression test that goes through SurveyDesign.resolve() with sum(raw_w) != n and checks scale invariance under a common rescaling of full and replicate weights. References: diff_diff/survey.py:192, diff_diff/survey.py:217, diff_diff/survey.py:1436, docs/methodology/REGISTRY.md:2009, diff_diff/staggered_aggregation.py:476, diff_diff/continuous_did.py:1233, diff_diff/efficient_did.py:1085, diff_diff/triple_diff.py:1095.
• Severity: P1. Impact: affected method is JKn replicate variance in both the OLS-refit and IF-based branches. The registry formula uses the original stratum replicate count n_h, but both shared JKn implementations recompute n_h from replicate_strata[valid] after invalid replicates are dropped. That reintroduces the design-factor error the previous review flagged whenever a JKn replicate is all-zero after subpopulation zeroing or an OLS replicate solve fails. Concrete fix: precompute original per-stratum replicate counts from resolved.replicate_strata once, keep those counts fixed in both JKn branches, and exclude invalid replicates only from the inner sum of squared differences. References: docs/methodology/REGISTRY.md:2008, diff_diff/survey.py:1392, diff_diff/survey.py:1487, diff_diff/survey.py:504.

Code Quality

No findings.

Performance

No findings.

Maintainability

No findings.

Tech Debt

• Severity: P3. Impact: TODO.md:55 says the replicate-df issue is resolved because n_replicates is updated to the valid count, but the shared metadata path still reports resolved.n_replicates / resolved.df_survey, and the summary renderer prints those fields directly. Some estimators patch inference df separately, but the shared metadata story is still not actually “resolved.” Concrete fix: either reopen/narrow the TODO entry or propagate effective n_valid into SurveyMetadata before results construction. References: TODO.md:55, diff_diff/survey.py:703, diff_diff/results.py:32, diff_diff/efficient_did.py:1093, diff_diff/triple_diff.py:586.

Security

No findings.

Documentation/Tests

• Severity: P2. Impact: the new suite checks finiteness and some helper behavior, but it does not cover the actual SurveyDesign.resolve() normalization path that triggers the P0 bug, and its JKn coverage stops at the all-valid case. The scale-invariance test constructs ResolvedSurveyDesign directly, which bypasses the normalization mismatch entirely. Concrete fix: add one helper-level regression that calls SurveyDesign.resolve() with non-unit-sum full weights and checks compute_replicate_if_variance() invariance to a common rescaling of full and replicate weights, and add JKn regressions where one replicate is empty/all-zero or one OLS replicate solve fails inside a stratum. References: tests/test_survey_phase6.py:524, tests/test_survey_phase6.py:567, tests/test_survey_phase6.py:590, tests/test_survey_phase6.py:771, tests/test_survey_phase6.py:921, tests/test_survey_phase6.py:938, tests/test_survey_phase6.py:989.

Path to Approval

1. Fix the combined_weights=True IF denominator so compute_replicate_if_variance() uses raw full-sample weights, not normalized resolved.weights, and add a regression test that goes through SurveyDesign.resolve() with sum(raw_w) != n.
2. Fix JKn dropped-replicate scaling in both compute_replicate_vcov() and compute_replicate_if_variance() to use the original per-stratum replicate counts, and add a regression test with one dropped/empty replicate inside a stratum.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: cb0880ad04d7d0ffcf47a47f17369f9b0b7c870e

---

Overall Assessment

⚠️ Needs changes

Executive Summary

• The prior P0/P1 issues from the last re-review look addressed: replicate IF variance now preserves raw full-sample weights, both JKn branches use original stratum replicate counts, ContinuousDiD event-study aggregation now honors anticipation, and EfficientDiD recomputes n_cl after EIF row filtering.
• Severity: P1. The new combined replicate-weight IF path still does not validate the core w_r / w_full assumption when zero full-sample weights are allowed. If combined_weights=True and any row has w_full = 0 but w_r > 0, the code silently substitutes 0 for an undefined ratio and returns wrong replicate SEs instead of erroring.
• Severity: P1 [Newly identified]. Zero-weight support was broadened to all survey weight types, but the survey TSL meat still ignores weights in the aweight branch. Zero-weight aweight rows therefore affect SEs even though they were excluded from estimation.
• Severity: P3. The new TODO entry claiming replicate survey d.f. is fully “resolved” is still overstated: inference d.f. is patched locally in several paths, but SurveyMetadata / summaries still report the original replicate count and R - 1.
• Static review only. I could not run the added tests here because this environment lacks both pytest and numpy.

Methodology

• Severity: P1. Impact: affected method is Replicate Weight Variance, specifically the IF-based combined_weights=True path used by CS aggregation, ContinuousDiD, EfficientDiD, and TripleDifference. The registry formula requires theta_r = sum((w_r / w_full) * psi), which is only defined when w_full > 0 wherever a combined replicate weight is positive. This PR explicitly allows zero full-sample weights, but SurveyDesign.resolve() does not enforce the needed consistency condition and compute_replicate_if_variance() silently replaces undefined ratios with zero via where=full_weights > 0. That can silently distort replicate SEs on malformed but currently accepted inputs. Concrete fix: in SurveyDesign.resolve() (or at minimum in compute_replicate_if_variance()), reject any combined_weights=True design where a row has weights == 0 and any replicate column is positive; add a regression test for that exact case. References: docs/methodology/REGISTRY.md:L2012, docs/methodology/REGISTRY.md:L2070, diff_diff/survey.py:L173, diff_diff/survey.py:L205, diff_diff/survey.py:L1450.
• Severity: P1 [Newly identified]. Impact: affected methods are the TSL survey variance paths used by OLS-based estimators through LinearRegression.fit(). The registry now documents zero-weight support for subpopulation analysis, and the PR’s weight validation changes allow zero aweights, but compute_survey_vcov() still builds aweight scores as X * residuals without any weight mask. A zero-weight aweight row is excluded from X'WX and coefficient estimation, yet it still enters the meat and changes the SE. That is incorrect variance output on a supported input class. Concrete fix: either make zero-weight aweight rows contribute zero to the survey meat (for example by masking them out in the aweight score path), or explicitly reject zero aweight survey designs and document that limitation in the registry; add a regression test showing that a zero-weight aweight row is inference-neutral relative to dropping that row entirely. References: docs/methodology/REGISTRY.md:L2070, diff_diff/survey.py:L1188, diff_diff/linalg.py:L403, diff_diff/linalg.py:L1792.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• Severity: P3. Impact: the PR marks replicate survey d.f. as fully resolved in TODO.md, but the code still does not push effective n_valid back into SurveyMetadata / n_replicates. Several inference paths patch d.f. locally, while summaries keep printing the original replicate count and R - 1, which is misleading but not numerically blocking. Concrete fix: either propagate effective replicate count into SurveyMetadata before results construction, or revert the TODO entry to an open limitation with a narrower description. References: TODO.md:L55, diff_diff/survey.py:L697, diff_diff/staggered_aggregation.py:L479, diff_diff/continuous_did.py:L1289, diff_diff/efficient_did.py:L1091, diff_diff/results.py:L32.

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: the new test coverage is broad, but it still misses the two remaining failure modes above: there is no regression asserting that inconsistent combined-weight replicate inputs are rejected, and no regression asserting that a zero-weight aweight row leaves survey SEs unchanged relative to dropping that row. Concrete fix: add one helper-level validation test for combined_weights=True with w_full=0 < w_r, and one estimator-level invariance test for zero-weight aweight survey inference. References: tests/test_survey_phase6.py:L178, tests/test_survey_phase6.py:L771.

Path to Approval

1. Enforce the combined-weight validity condition for IF-based replicate variance: when combined_weights=True, reject any row with full_weight == 0 and a positive replicate weight. Add a regression test that currently passes the malformed design and should raise ValueError.
2. Fix the zero-weight aweight survey variance path so zero-weight rows do not enter the TSL meat, or explicitly reject zero aweight survey designs and document that limitation. Add a regression test comparing a fit with a zero-weight aweight row against the same fit with that row dropped, and require identical SEs.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: aefa7f380d74633f7d8230f7c208355a4f20c962

---

Overall assessment

⚠️ Needs changes

Executive summary

• The prior re-review blockers look fixed in the diff: the combined-weight IF path now rejects w_r > 0 when w_full = 0, zero-weight aweight rows are masked out of the TSL meat, EfficientDiD recomputes clustered n_cl after EIF filtering, and ContinuousDiD event-study aggregation now honors anticipation.
• Severity: P1. The new replicate-weight implementation hardcodes survey degrees of freedom as R - 1, but the reference survey::svrepdesign() API computes replicate-design d.f. from degf(), i.e. from the numerical rank of the replicate-weight matrix. That is especially wrong for JKn, which is a stratified delete-one-cluster jackknife, so current p-values/CIs can be anti-conservative. (search.r-project.org)
• Severity: P1. SurveyDesign(..., mse=True) and the registry’s documented default do not match survey::svrepdesign(), whose mse default comes from options("survey.replicates.mse"), and that package default is FALSE. Omitting mse therefore changes replicate SEs relative to the stated source behavior. (search.r-project.org)
• Severity: P1. [Newly identified] Zero-weight support was only partially rolled out: manual WLS paths in ContinuousDiD and TripleDifference still key off raw row counts, not positive-weight rows, so subpopulation zero weights can push them into rank-dropped fits instead of a clean skip/error.
• Severity: P3. TODO.md still marks replicate-weight survey d.f. as resolved, but the implementation is still using column-count d.f., not rank-based d.f.
• Static review only. I could not run the test suite here because this environment has neither pytest nor numpy.

Methodology

• Severity: P1. Impact: replicate-weight inference currently uses df_survey = n_replicates - 1 in the core design object and then threads that through OLS- and IF-based inference paths via diff_diff/survey.py:L568-L571, diff_diff/linalg.py:L1801-L1868, and docs/methodology/REGISTRY.md:L2014-L2042. The reference survey docs say replicate-design d.f. come from degf(), which is computed from the numerical rank of the replicate weights by default; JKn is explicitly the stratified delete-one-cluster jackknife, so R - 1 is not generally the right denominator d.f. Concrete fix: compute/store replicate d.f. as rank(repweights) - 1 (or support an explicit degf override mirroring svrepdesign()), propagate that through SurveyMetadata, LinearRegression, and the IF-based estimators, and add a JKn regression test where rank-based d.f. is strictly below R - 1. (search.r-project.org)
• Severity: P1. Impact: the new public default mse=True in diff_diff/survey.py:L65-L72 and docs/methodology/REGISTRY.md:L2017-L2024 is an undocumented deviation from survey::svrepdesign(), whose default is mse=getOption("survey.replicates.mse") and whose package default option is FALSE. That changes replicate SEs whenever callers omit mse, so the new API does not actually match the stated R default behavior. Concrete fix: either change the library default to False, or keep True but mark it explicitly as a **Note (deviation from R):** in the registry/docs and add tests covering the default plus both mse branches. (search.r-project.org)

Code Quality

• Severity: P1. Impact: [Newly identified] the PR added zero-weight support centrally, but the manual WLS fit paths in diff_diff/continuous_did.py:L914-L935 and diff_diff/triple_diff.py:L1191-L1223 still decide whether a cell is estimable from raw row counts and then fit on sqrt-weighted matrices. After subpopulation zeroing, a cell can have enough rows but too few positive-weight observations to identify the requested spline/covariate regression, and these paths then proceed with rank-dropped coefficients rather than skipping/failing on the effective sample. Concrete fix: base the pre-fit checks on positive-weight observations (and, ideally, positive-weight design rank), or route these paths through a shared weighted-fit helper that validates the effective sample before fitting.

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• Severity: P3. Impact: TODO.md:L55-L55 marks replicate-weight survey d.f. as resolved, but the code still uses count-based rather than rank-based d.f. Concrete fix: if the d.f. bug above is fixed, keep the entry resolved; otherwise revert it to an open item with the narrower, accurate description. (search.r-project.org)

Security

• No findings.

Documentation/Tests

• Severity: P2. Impact: the new tests cover JK1/BRR/Fay variance mechanics, but they do not cover the two methodology mismatches above or the zero-weight effective-sample edge case. Existing replicate d.f. assertions in tests/test_survey_phase6.py:L399-L499 only encode R - 1, and the JKn coverage in tests/test_survey_phase6.py:L551-L588 never checks inference d.f. Concrete fix: add one JKn d.f. test with multiple strata, one default-mse parity test versus explicit mse=False, and one zero-weight subpopulation regression test for each manual WLS path that currently relies on raw row counts.

Path to Approval

1. Replace replicate df_survey = R - 1 with rank-based replicate d.f. (or an explicit degf override), and add a JKn regression test where the expected d.f. is below R - 1.
2. Align the default mse behavior with survey::svrepdesign() or explicitly document the deviation with a Note (deviation from R) label in the registry/docs and tests.
3. Harden the manual WLS paths in ContinuousDiD and TripleDifference so zero-weight observations do not count toward identifiability, and add regression tests where the positive-weight sample is too small for the requested model.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 3efa276f4b3acfb34a2ea672b62379701c1efad0

---

Overall Assessment

⛔ Blocker

Most of the prior re-review blockers look fixed: mse now defaults to False in diff_diff/survey.py:72, EfficientDiD recomputes the effective clustered n_cl after EIF filtering in diff_diff/efficient_did.py:1597, and ContinuousDiD event-study aggregation now honors anticipation in diff_diff/continuous_did.py:1075. One new P0 remains in the replicate-variance implementation, and one prior P1 is still unresolved in the manual WLS cell fits.

Executive Summary

• The prior mse default mismatch is fixed, the base replicate df_survey property is now rank-based, the EfficientDiD Hausman n_cl bug is fixed, and ContinuousDiD now filters contaminated pre-periods in event-study aggregation.
• P0: dropped or zero-mass replicate columns are excluded from the replicate-variance sums but still scaled by the original replicate count / original JKn stratum counts, which understates SEs; the IF path does this silently after subpopulation zeroing.
• P1: ContinuousDiD and TripleDifference still decide WLS cell identifiability from raw row counts instead of positive-weight rows, so subpopulation zero weights can still push those paths into rank-dropped fits instead of a clean skip/error.
• P3: Phase 6 docs/tests still encode stale replicate-d.f. behavior (R-1 / n_valid-1) even though the main registry rule is now rank-based.
• Static review only. I could not run the tests here because this environment does not have pytest or numpy.

Methodology

• P0 Impact: the new replicate-weight variance code says invalid replicates are dropped and the registry says variance is computed from valid replicates only, but both compute_replicate_vcov() and compute_replicate_if_variance() still apply BRR/Fay/JK1/JKn scaling with the original replicate counts after filtering to valid. Because SurveyDesign.subpopulation() now zeros excluded replicate columns, a domain can leave some replicate columns entirely zero, compute_replicate_if_variance() will drop them via valid = np.isfinite(theta_reps), and then return an anti-conservative SE with no warning as long as at least two replicates remain. Relevant code: diff_diff/survey.py:508, diff_diff/survey.py:1349, diff_diff/survey.py:1424, docs/methodology/REGISTRY.md:2031. Concrete fix: once invalid replicates are removed, recompute BRR/Fay/JK1 scaling on n_valid and JKn scaling on valid per-stratum counts, or fail hard when any replicate is lost; also emit an explicit warning in the IF path whenever n_valid < R.

Code Quality

• P1 Impact: the prior positive-weight effective-sample issue is still unresolved in the manual WLS cell fits. ContinuousDiD still gates on raw n_treated before fitting and then solves on preweighted Psi_w / delta_tilde_y_w, and TripleDifference still does the same in _fit_predict_mu(). After subpopulation zeroing, these paths can have enough raw rows but too few positive-weight observations to identify the requested regression, and they proceed with rank-dropped fits or fallback means instead of a clean skip/error on the effective sample. Relevant code: diff_diff/continuous_did.py:914, diff_diff/continuous_did.py:925, diff_diff/triple_diff.py:1191, diff_diff/triple_diff.py:1201. Concrete fix: validate w > 0 count and rank before these fits, or route both paths through a shared weighted-fit helper that accepts untransformed X, y, and weights and refuses unidentifiable positive-weight samples.

Performance

• No findings.

Maintainability

• No findings.

Tech Debt

• P3 Impact: the Phase 6 replicate-d.f. story is still internally inconsistent across project docs and tracking. The methodology registry’s main rule is rank-based, but the roadmap still says Survey df = R-1, and TODO.md still claims the issue is resolved via df_survey = n_valid - 1. Relevant docs: docs/methodology/REGISTRY.md:2014, docs/survey-roadmap.md:102, TODO.md:55. Concrete fix: after settling the dropped-replicate behavior, align the registry note, roadmap, and TODO entry to one rule.

Security

• No findings.

Documentation/Tests

• P2 Impact: the new Phase 6 tests only exercise full-valid replicate designs and still encode df_survey == len(rep_cols) - 1, so they will not catch either the dropped-replicate scaling bug or a rank-based/JKn case where df < R-1. Relevant tests: tests/test_survey_phase6.py:399, tests/test_survey_phase6.py:411, tests/test_survey_phase6.py:486, tests/test_survey_phase6.py:499. Concrete fix: add one subpopulation test that zeros entire replicate columns and checks the manual valid-only variance, one singular-replicate OLS test, and one JKn test where the expected replicate d.f. is strictly below R-1.

Path to Approval

1. Fix dropped-replicate scaling in both replicate-variance functions so BRR/Fay/JK1/JKn use the surviving replicate set consistently, and warn or fail when n_valid < R.
2. Make ContinuousDiD and TripleDifference validate the positive-weight effective sample before WLS cell fits instead of relying on raw row counts.
3. Add regression tests for the two cases above, then align the Phase 6 docs/tests/TODO text with the final replicate-d.f. behavior.