Skip to content

refactor(survey): consolidate panel-to-unit collapse (ContinuousDiD/EfficientDiD)#602

Open
igerber wants to merge 1 commit into
mainfrom
refactor/survey-unit-collapse-consolidation
Open

refactor(survey): consolidate panel-to-unit collapse (ContinuousDiD/EfficientDiD)#602
igerber wants to merge 1 commit into
mainfrom
refactor/survey-unit-collapse-consolidation

Conversation

@igerber

@igerber igerber commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • Consolidate the "row-index → unit-level" survey-design collapse that ContinuousDiD and EfficientDiD each hand-rolled. New diff_diff/survey.py helpers: ResolvedSurveyDesign.subset_to_units_by_row_idx(row_idx, unit_weights=None) (folds the index-and-recount preamble around the existing subset_to_units) and build_unit_first_row_index(unit_values, unit_order) (first-panel-row index per unit).
  • ContinuousDiD: 3 collapse sites (event-study / overall-dose / bootstrap SE) + a slow df.iterrows() index build → single helper calls. EfficientDiD: index build + build-once collapse → helper calls. Net ~95 fewer lines of duplicated collapse.
  • StackedDiD is deliberately left on its own path (control units are duplicated across sub-experiments, so it re-resolves at stacked-row granularity rather than collapsing to one row per unit) — clarifying comment only; the residual stacked-specific dedup is parked in TODO.md.
  • Bit-identical: same np.unique + subset_to_units + arrays; survey-weighted SEs, df_survey, and design-effect metadata are unchanged.

Methodology references (required if estimator / math changes)

  • Method name(s): ContinuousDiD (CGBS 2024) and EfficientDiD (Chen-Sant'Anna-Xie 2025) survey-weighted IF/EIF variance paths — internal refactor only; no estimand, variance formula, or weighting behavior changes.
  • Paper / source link(s): N/A (no methodology change). The unit-level survey collapse this consolidates is the same one the REGISTRY survey-variance sections already describe.
  • Any intentional deviations from the source (and why): None. This is a pure code-organization change; SEs / metadata are bit-identical (see Validation).

Validation

  • Tests added/updated: tests/test_survey.py::TestUnitCollapseHelpers (4 tests) — build_unit_first_row_index on an unsorted panel, and subset_to_units_by_row_idx element-for-element equal to the OLD inline preamble + subset_to_units (the frozen oracle), covering both the explicit-unit_weights and default (None) paths plus a replicate-weight design (R-column subset preserved).
  • Regression (the real guarantee): the existing ContinuousDiD + EfficientDiD survey suites pass unchanged — tests/test_survey.py, tests/test_methodology_continuous_did.py, tests/test_efficient_did.py (329 passed locally). No golden re-baselining.
  • mypy: net −6 errors (the one-line wrapper collapses each site's pre-existing object-attr errors), zero new.

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@igerber @claude
refactor(survey): consolidate panel-to-unit collapse (ContinuousDiD/E…
98ce01e 
…fficientDiD)

Extract the row_idx->unit survey-design collapse hand-rolled at four sites
(continuous_did.py event-study/overall/bootstrap SE + efficient_did.py
build-once) into two diff_diff/survey.py helpers:

- ResolvedSurveyDesign.subset_to_units_by_row_idx(row_idx, unit_weights=None):
  folds the index-and-recount preamble around the existing subset_to_units.
- build_unit_first_row_index(unit_values, unit_order): first-panel-row index
  per unit (replaces ContinuousDiD's slow df.iterrows() build and
  EfficientDiD's inline .values scan).

Bit-identical: same np.unique + subset_to_units + arrays; survey-weighted SEs,
df_survey, and design-effect metadata are unchanged (oracle test locks it vs
the old inline logic). Removes ~95 lines of duplicated collapse.

StackedDiD is deliberately left on its own path (control units are duplicated
across sub-experiments, so it re-resolves at stacked granularity rather than
collapsing to one row per unit) with a clarifying comment; the residual
stacked-specific dedup is parked in TODO.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected methods: ContinuousDiD and EfficientDiD survey-weighted unit-level IF/EIF variance paths; StackedDiD gets only a clarifying comment/TODO.
  • The new subset_to_units_by_row_idx() helper preserves the old row-index collapse: weights, strata, PSU, FPC, replicate-weight rows, and recounted unit-level design dimensions.
  • Cross-check against the registry found no undocumented methodology deviation from the documented survey TSL, survey df, bootstrap, or replicate-weight contracts.
  • No new inline inference anti-pattern or partial NaN guard was introduced in the changed code.
  • Added helper tests cover unsorted row ordering, explicit/default weights, and replicate-row preservation.
  • I could not run pytest in this review environment: pytest and numpy are not installed.

Methodology

Finding: No unmitigated methodology findings.
Severity: N/A
Impact: The affected survey variance paths remain consistent with the registry: ContinuousDiD survey support is TSL on influence functions plus survey bootstrap support, and EfficientDiD survey support is TSL on EIF scores with unit-level df threading. See docs/methodology/REGISTRY.md:L952-L953, docs/methodology/REGISTRY.md:L1133-L1144, docs/methodology/REGISTRY.md:L4328-L4343, docs/methodology/REGISTRY.md:L4388-L4400, and docs/methodology/REGISTRY.md:L4460-L4485.
Concrete fix: None.

Code Quality

Finding: No code quality findings.
Severity: N/A
Impact: ResolvedSurveyDesign.subset_to_units_by_row_idx() cleanly centralizes the prior repeated preamble and delegates to subset_to_units(), preserving replicate metadata. See diff_diff/survey.py:L685-L720. Call sites in ContinuousDiD and EfficientDiD are narrowly scoped.
Concrete fix: None.

Performance

Finding: No performance findings.
Severity: N/A
Impact: Replacing the ContinuousDiD iterrows() first-row index build with build_unit_first_row_index() is a straightforward improvement without changing estimator semantics. See diff_diff/continuous_did.py:L821-L825 and diff_diff/survey.py:L1138-L1155.
Concrete fix: None.

Maintainability

Finding: No maintainability findings.
Severity: N/A
Impact: The shared helper removes duplicate survey-collapse logic while keeping StackedDiD separate where duplicated control units require stacked-granularity resolution. See diff_diff/stacked_did.py:L774-L780.
Concrete fix: None.

Tech Debt

Finding: Residual StackedDiD intra-file survey dedup remains, but is explicitly tracked.
Severity: P3 informational
Impact: This is documented in TODO.md as low-priority, stacked-specific follow-up work and is not a blocker under the deferred-work policy. See TODO.md:L123-L125.
Concrete fix: No PR-blocking fix required.

Security

Finding: No security findings.
Severity: N/A
Impact: The diff is internal Python refactoring/tests/docs only; no secrets, credential handling, network calls, or unsafe deserialization paths were introduced.
Concrete fix: None.

Documentation/Tests

Finding: Verification environment missing test dependencies.
Severity: P3 informational
Impact: The PR adds targeted tests at tests/test_survey.py:L3695-L3787, but I could not execute them locally because pytest and numpy are unavailable in the review environment.
Concrete fix: Ensure CI runs tests/test_survey.py::TestUnitCollapseHelpers plus the existing ContinuousDiD/EfficientDiD survey suites.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@igerber