Skip to content

test(survey): true half-sample BRR replicate regressions per estimator family#648

Open
igerber wants to merge 1 commit into
mainfrom
test/true-brr-replicate-regressions
Open

test(survey): true half-sample BRR replicate regressions per estimator family#648
igerber wants to merge 1 commit into
mainfrom
test/true-brr-replicate-regressions

Conversation

@igerber

@igerber igerber commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • The replicate-weight expansion tests used Fay-like 0.5/1.5 perturbations, under which every unit keeps positive weight; true BRR was covered only at the vcov-helper level (test_survey_phase6.py). This adds a Hadamard-balanced half-sample generator — paired 2-PSU pseudo-strata; within each pair the selected PSU gets w*2, the other exactly 0 (Wolter 2007 ch. 3; the survey::brrweights full-BRR convention with combined weights). Strata map to Hadamard columns 1..H (the constant column 0 is skipped, as brrweights does — using it would leave the first stratum permanently unbalanced; a construction-sanity test asserts exact half-sampling {0,2} AND per-PSU balance: mean multiplier exactly 1 across replicates).
  • New TestTrueBRRHalfSample regression class per estimator family (DiD, DiD-absorb, MultiPeriodDiD, TWFE, SunAbraham, StackedDiD, ImputationDiD, TwoStageDiD): finite positive replicate SEs under genuine half-samples, plus a base-weights point-estimate invariance check where applicable (replicate columns drive only the variance).
  • Real behavior exercised for the first time: genuine half-samples CAN lose identification inside a replicate refit on some designs (the staggered binary-interaction TWFE parameterization) — TWFE fails loudly there rather than silently degrading; noted in-test, and the family test uses its dedicated 2-period panel. Closes the Add replicate weight support to 7 estimators #253 TODO row.

Methodology references (required if estimator / math changes)

  • Method name(s): Balanced repeated replication (Wolter 2007 ch. 3; R survey::brrweights conventions) — tests only, no estimator code changed
  • Paper / source link(s): REGISTRY.md replicate-weight variance sections (BRR 1/R factor, combined-weights contract)
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: tests/test_replicate_weight_expansion.py (_add_true_brr_replicates + 9 tests; full file 31 pass).
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • PR is test/docs/TODO-only; no estimator math or defaults changed.
  • Affected coverage: BRR replicate-weight paths for DiD, DiD absorb, MultiPeriodDiD, TWFE, SunAbraham, StackedDiD, ImputationDiD, and TwoStageDiD.
  • The Hadamard half-sample generator is consistent with the registry’s BRR combined-weight contract: raw replicate columns, BRR 1/R scaling, and documented invalid-replicate handling.
  • TODO removal is appropriate: the prior true half-sample BRR coverage row is now addressed.
  • I could not run tests locally because pytest is not installed in this environment.

Methodology
No findings.

Cross-check: docs/methodology/REGISTRY.md:L4783-L4860 documents BRR/Fay/JK replicate variance, raw combined replicate weights, BRR 1/R, and invalid replicate handling. The new helper at tests/test_replicate_weight_expansion.py:L527-L568 generates combined full-sample replicate weights with {0, 2} multipliers for paired PSUs, which matches that contract.

Code Quality
No findings.

Performance
No findings. The helper is test-only and uses small deterministic fixtures.

Maintainability
No findings.

Tech Debt
No findings. The removed TODO row at TODO.md:L63-L68 corresponds to the new per-family true BRR regression coverage.

Security
No findings. No secrets or external I/O introduced.

Documentation/Tests

  • Severity: P3
  • Finding: The changelog/test prose slightly overstates point-estimate invariance coverage. CHANGELOG.md:L58-L60 says the per-family class includes a base-weights point-estimate invariance check, but the new class only asserts invariance for DiD, TWFE, SunAbraham, and ImputationDiD; DiD-absorb, MultiPeriodDiD, StackedDiD, and TwoStageDiD only assert finite positive SEs (tests/test_replicate_weight_expansion.py:L626-L653, L694-L727).
  • Impact: Minor documentation/test clarity issue; no correctness risk in estimator code.
  • Concrete fix: Either add base-fit invariance assertions for the remaining applicable families, or qualify the changelog/test prose with “where checked/applicable.”

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 7, 2026
…r family

The replicate-weight expansion tests used Fay-like 0.5/1.5
perturbations, under which every unit keeps positive weight; true BRR
was covered only at the vcov-helper level (test_survey_phase6). Adds a
Hadamard-balanced half-sample generator (paired 2-PSU pseudo-strata;
selected PSU w*2, the other exactly 0 — the survey::brrweights full-BRR
convention) and a per-family regression class (DiD, DiD-absorb,
MultiPeriodDiD, TWFE, SunAbraham, StackedDiD, ImputationDiD,
TwoStageDiD) asserting finite positive replicate SEs under genuine
half-samples plus base-weights point-estimate invariance (replicate
columns drive only the variance). Construction sanity asserted (half
the paired PSUs zeroed per replicate; multipliers in {0,2}). Genuine
half-samples CAN lose identification inside a replicate refit on some
designs (staggered binary-interaction TWFE) — TWFE fails loudly there;
noted in-test with the family's dedicated panel.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@igerber igerber force-pushed the test/true-brr-replicate-regressions branch from 3e2225f to 2944e23 Compare July 8, 2026 02:22
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 2944e23f92ac0bf8d191a07ba5bb4ae4aabcc416


Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • This PR remains test/docs/TODO-only; no estimator math, defaults, or inference code changed.
  • Affected coverage is BRR replicate-weight paths for DiD, DiD-absorb, MultiPeriodDiD, TWFE, SunAbraham, StackedDiD, ImputationDiD, and TwoStageDiD.
  • The true-BRR helper matches the registry contract for raw combined replicate weights and half-sample zero-weight PSU behavior.
  • Prior P3 documentation issue is partially fixed in the test docstring, but still present in CHANGELOG.md.
  • I could not run the added tests because pytest is not installed in this environment.

Methodology

No findings.

Cross-check: docs/methodology/REGISTRY.md:L4780-L4821 documents BRR replicate variance, combined replicate weights, and preserving raw replicate columns; docs/methodology/REGISTRY.md:L4824-L4831 documents invalid-replicate handling; docs/methodology/REGISTRY.md:L4927-L4933 documents zero-weight observations being excluded during estimator-level replicate refits. The new helper at tests/test_replicate_weight_expansion.py:L527-L568 creates combined full-sample replicate columns with {0, 2} multipliers, which is consistent with that contract.

Code Quality

No findings.

Performance

No findings. The added work is test-only and uses small deterministic fixtures.

Maintainability

No findings.

Tech Debt

No findings. Removing the TODO row is appropriate because the new TestTrueBRRHalfSample class covers the referenced per-family true half-sample BRR regression gap.

Security

No findings. No secrets, network access, or external I/O introduced.

Documentation/Tests

  • Severity: P3
  • Impact: The previous documentation clarity issue is only partially resolved. The test class docstring now correctly says point-estimate invariance is checked “where checked” (tests/test_replicate_weight_expansion.py:L576-L580), but the changelog still says the per-family class includes “finite positive replicate SEs ... plus a base-weights point-estimate invariance check” after listing all families (CHANGELOG.md:L70-L73). In practice, invariance is asserted for DiD, TWFE, SunAbraham, and ImputationDiD, but not DiD-absorb, MultiPeriodDiD, StackedDiD, or TwoStageDiD (tests/test_replicate_weight_expansion.py:L626-L653, L694-L727).
  • Concrete fix: Qualify the changelog with “where checked/applicable,” or add base-fit invariance assertions for the remaining applicable families.

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

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant