refactor(spillover): route stage-1 FE through shared _iterative_fe_solve

[igerber]

Summary

• spillover._iterative_fe_subset becomes a thin Butts-subsample wrapper over diff_diff.utils._iterative_fe_solve — the shared bincount Gauss-Seidel engine ImputationDiD/TwoStageDiD already route through — taking the library's FE-solver copy count from 2 to 1 (the TODO row's goal).
• The wrapper keeps the SpilloverDiD-specific front door: the empty-Omega_0 and empty positive-weight-Omega_0 ValueError gates and the subsample extraction. The shared engine owns the iteration, the zero-weight/positive-weight NaN-FE convention (identical contract, per the existing cross-references in REGISTRY), and the warn_if_not_converged non-convergence UserWarning — now labelled "SpilloverDiD stage-1 FE (Butts Omega_0 subsample)", replacing the caller-side warning.
• Per sweep the shared engine computes the identical group means and convergence metric as the historical local loop, so converged fits are bit-identical (all 319 spillover tests, including the Wave B-E golden/parity suites, pass unchanged).
• max_iter aligned from the historical local cap of 100 to the shared 10,000 convention (the fixest/pyfixest budget; the option the TODO row offered "with a REGISTRY note") — fits that previously exhausted 100 sweeps and warned may now converge instead (strictly more accurate FE; tol=1e-10 unchanged). New REGISTRY SpilloverDiD Note documents the routing and the budget alignment.

Methodology references (required if estimator / math changes)

• Method name(s): Butts (2021) spillover DiD, Gardner (2022) two-stage stage-1 FE estimation (solver routing only; the Omega_0 identification contract, NaN-FE convention, and connectivity checks are unchanged)
• Paper / source link(s): REGISTRY.md § SpilloverDiD (new "shared FE engine" Note; existing Omega_0 notes unchanged)
• Any intentional deviations from the source (and why): None — the alternating-projection recursion is the same; only its implementation home and iteration budget changed (documented).

Validation

• Tests added/updated: none needed — tests/test_spillover.py (319 tests incl. golden/parity/survey suites) and tests/test_methodology_two_stage.py pass unchanged, pinning converged-fit bit-identity.
• 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

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• No methodology mismatch found for SpilloverDiD stage-1 FE routing.
• The affected method is Butts/Gardner-style SpilloverDiD stage-1 fixed-effect estimation on Omega_0 = {D_it = 0 AND S_it = 0}.
• The max_iter change from 100 to 10_000 is explicitly documented in docs/methodology/REGISTRY.md:L3992, so it is P3 informational, not a defect.
• Empty Omega_0 and positive-weight Omega_0 gates are preserved in diff_diff/spillover.py:L1455-L1481.
• Zero-weight NaN-FE behavior is delegated to the shared helper and covered by existing tests in tests/test_utils.py:L2048-L2065 and tests/test_spillover.py:L5165-L5218.
• I could not run tests locally because pytest and numpy are not installed in this environment.

Methodology

Finding: P3 informational — documented solver-budget behavior change

• Impact: SpilloverDiD stage-1 FE solves can now continue up to 10_000 iterations rather than warning after 100. This may change non-converged historical fits by letting them converge, but the registry documents the behavior and frames it as alignment with the shared FE engine.
• Evidence: _FE_ITER_MAX = 10_000 in diff_diff/spillover.py:L1289-L1294; registry note in docs/methodology/REGISTRY.md:L3992.
• Concrete fix: No action required.

No P0/P1 methodology defects found. The source-material contract is preserved: Butts’ staggered spillover estimator estimates stage-1 FE on untreated-and-unexposed observations, and Gardner’s two-stage framework identifies group/time effects from untreated observations before residualized stage 2. The PR only changes the numerical engine used for that same alternating FE solve.

Code Quality

Finding: None.

The wrapper remains narrow and preserves Spillover-specific validation before delegating to _iterative_fe_solve: diff_diff/spillover.py:L1396-L1510. The caller correctly handles the new two-value return at diff_diff/spillover.py:L2656-L2662.

Performance

Finding: None.

The shared helper uses per-sweep np.bincount passes over integer codes, matching the intended consolidation and avoiding the removed per-row local loop. See diff_diff/utils.py:L3053-L3092.

Maintainability

Finding: None.

Removing the duplicate FE loop reduces maintenance surface. The TODO row for migrating spillover._iterative_fe_subset was removed from the actionable backlog, consistent with the implemented routing.

Tech Debt

Finding: P3 informational — tracked deferred item completed

• Impact: The PR removes the TODO row for migrating spillover._iterative_fe_subset to the shared solver, which is consistent with the implementation.
• Evidence: TODO.md:L56-L59 no longer includes the migration row, and the wrapper delegates at diff_diff/spillover.py:L1500-L1510.
• Concrete fix: No action required.

Security

Finding: None.

No secrets, unsafe I/O, network access, or security-sensitive behavior changed.

Documentation/Tests

Finding: None requiring changes.

Documentation was updated in CHANGELOG.md and docs/methodology/REGISTRY.md:L3992. Existing tests cover the shared FE solver parity, zero-weight NaN-FE behavior, non-convergence warning label, and Spillover zero-weight Omega_0 handling: tests/test_utils.py:L2032-L2081, tests/test_spillover.py:L5165-L5218.

Tests not run: pytest was unavailable, and the environment also lacks numpy, so I could not execute the targeted test subset locally.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 438478a47657dd058bc7911e7835c28c4ed93376

---

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Re-review found no methodology regression in the SpilloverDiD FE routing.
• Affected estimator: SpilloverDiD stage-1 Butts Omega_0 FE solve, with Gardner-style residualization.
• The max_iter change from 100 to 10_000 is explicitly documented in REGISTRY.md, so it is P3 informational.
• The Callaway/Sant’Anna IPW fixture/test deferral is tracked in TODO.md, and no estimator code changed there.
• Could not run tests locally: python -m pytest failed because pytest is not installed.

Methodology

Finding: P3 informational — documented SpilloverDiD solver-budget change

• Impact: Fits that previously warned after 100 FE sweeps may now continue to 10,000 and converge. This is a default-behavior change, but it is documented as intentional.
• Evidence: diff_diff/spillover.py:L1289-L1294, docs/methodology/REGISTRY.md:L3992-L3996.
• Concrete fix: No action required.

Finding: P3 informational — Callaway no-covariate IPW structural parity remains deferred/tracked

• Impact: The PR removes the more explicit registry note/test for mirroring R’s intercept-only logit path, but the remaining registry note still documents unconditional no-covariate IPW, and TODO.md tracks the structural-parity decision.
• Evidence: docs/methodology/REGISTRY.md:L522-L549, TODO.md:L34.
• Concrete fix: No action required for approval; eventually either keep the documented deviation or mirror R structurally.

Code Quality

Finding: None.

The _iterative_fe_subset return-shape change is fully propagated; the private helper is only called by SpilloverDiD.fit, now expecting the two-array return. See diff_diff/spillover.py:L1396-L1510 and diff_diff/spillover.py:L2650-L2663.

Performance

Finding: None.

Routing through _iterative_fe_solve uses the shared bincount Gauss-Seidel engine and removes the duplicate local loop. See diff_diff/utils.py:L2985-L3094.

Maintainability

Finding: P3 informational — IPW aggregation fixture regeneration is intentionally deferred

• Impact: The R generator emits IPW aggregation blocks, while the checked-in JSON omits them and the test hardcodes the R values. This is less maintainable than fixture-driven assertions, but it is explicitly tracked.
• Evidence: benchmarks/R/generate_csdid_test_values.R:L111-L140, tests/test_csdid_ported.py:L1484-L1535, TODO.md:L35.
• Concrete fix: No action required for approval; on next fixture regeneration, commit the JSON aggregation blocks and read expected values from JSON again.

Tech Debt

Finding: P3 informational — completed spillover FE-solver TODO removed

• Impact: The prior actionable item to migrate spillover._iterative_fe_subset to the shared solver is resolved by this PR.
• Evidence: diff_diff/spillover.py:L1500-L1510; the old TODO row is removed from the performance backlog.
• Concrete fix: No action required.

Security

Finding: None.

No secrets, unsafe I/O, network calls, or security-sensitive behavior changed.

Documentation/Tests

Finding: P3 informational — targeted tests not run locally

• Impact: Static review found the relevant parity/zero-weight tests in-tree, but I could not execute them in this environment.
• Evidence: attempted python -m pytest ...; failed with No module named pytest.
• Concrete fix: Run the targeted CI slice in an environment with test dependencies installed.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 01f4c72c507593397e81cf1a9305ac72f4fa22e3

---

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected method: SpilloverDiD stage-1 FE solve on Butts Omega_0, used before Gardner-style residualization.
• The wrapper preserves empty Omega_0, positive-weight Omega_0, and connectivity checks before delegating to _iterative_fe_solve.
• The max_iter default change from 100 to 10_000 is documented in REGISTRY.md, so it is P3 informational, not a defect.
• No new inline inference, NaN/Inf propagation, parameter propagation, or control-group issues found.
• Local tests could not run: this environment lacks pytest and numpy; git diff --check passed.

Methodology

Finding: P3 informational — documented SpilloverDiD FE budget/routing change

• Impact: Stage-1 fits that previously warned after 100 sweeps may now continue to 10,000 sweeps and converge. This is a default-behavior change, but it is explicitly documented with a REGISTRY Note.
• Evidence: diff_diff/spillover.py:L1289-L1294, diff_diff/spillover.py:L1396-L1510, diff_diff/utils.py:L2985-L3094, docs/methodology/REGISTRY.md:L3998-L4002.
• Concrete fix: No action required.

Code Quality

Finding: None.

The _iterative_fe_subset return-shape change is propagated to its only in-tree caller. See diff_diff/spillover.py:L2650-L2663.

Performance

Finding: None.

The shared solver removes the duplicate local FE loop. The longer iteration budget can increase runtime only for hard non-convergent cases, and that behavior is documented as intentional.

Maintainability

Finding: P3 informational — shared-helper docstring is slightly stale

• Impact: _iterative_fe_solve still describes SpilloverDiD._iterative_fe_subset as a “structural sibling,” while this PR makes it a wrapper over the helper. This is documentation drift only.
• Evidence: diff_diff/utils.py:L2999-L3006, diff_diff/spillover.py:L1413-L1424.
• Concrete fix: Update the helper docstring to say SpilloverDiD now routes through _iterative_fe_solve.

Tech Debt

Finding: None.

The completed TODO row for migrating spillover._iterative_fe_subset to the shared solver was removed consistently. See TODO.md:L44-L56.

Security

Finding: None.

No secrets, unsafe I/O, network behavior, or security-sensitive code paths changed.

Documentation/Tests

Finding: P3 informational — tests not executable in this review environment

• Impact: I could not run the targeted test slice because pytest and numpy are not installed locally. Static review and git diff --check completed.
• Concrete fix: Run the relevant CI slice in a dependency-complete environment, especially tests/test_utils.py::TestIterativeFeSolve and the Spillover zero-weight Omega_0 tests.