Skip to content

perf(callaway): fancy-+= per-(g,t) IF scatters; drop dead _compute_aggregated_se#636

Merged
igerber merged 1 commit into
mainfrom
perf/cs-if-scatter-cleanup
Jul 7, 2026
Merged

perf(callaway): fancy-+= per-(g,t) IF scatters; drop dead _compute_aggregated_se#636
igerber merged 1 commit into
mainfrom
perf/cs-if-scatter-cleanup

Conversation

@igerber

@igerber igerber commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • Convert the per-(g,t) IF scatters in staggered.py::_cluster_robust_se_from_per_gt_if and the general combined-IF assembly path in staggered_aggregation.py from np.add.at (unbuffered ufunc, 5-20x slower per element) to fancy +=. Index arrays are duplicate-free by construction at every producer (np.where on disjoint masks — the documented invariant _combined_if_fast already relies on), so the fancy scatter is bit-identical.
  • Remove the dead _compute_aggregated_se (zero in-package callers; superseded by _compute_aggregated_se_with_wif since the WIF adjustment landed) and update the stale test_se_accuracy.py docstring pointer.
  • Refresh the REGISTRY note + fast-path dispatch comment that described the general fallback as "preserved verbatim/unchanged" (now: same mathematical contract, fancy-+= scatter).
  • Closes the two CS-scaling Quick TODO rows (np.add.at scatter + dead-code removal); CHANGELOG entry under Unreleased → Performance.

Methodology references (required if estimator / math changes)

  • Method name(s): Callaway & Sant'Anna (2021) group-time ATT influence-function aggregation (no formula change — scatter mechanics only)
  • Paper / source link(s): REGISTRY.md § CallawaySantAnna (combined-IF assembly Note)
  • Any intentional deviations from the source (and why): None. np.add.at and fancy += are bit-identical on duplicate-free index arrays (each element receives exactly one add of the same two floats).

Validation

  • Tests added/updated: tests/test_se_accuracy.py (docstring pointer only). Existing gates cover the change: tests/test_staggered_aggregation.py::TestCombinedIFFrozenParity (frozen pre-rewrite oracle over panel / survey / RCS / unbalanced / triple-diff scenarios) and tests/test_staggered.py::TestCallawaySantAnnaClusterSafetyGates::test_per_gt_se_matches_compute_survey_if_variance_helper (exact rel=0, abs=0 equality vs an np.add.at reconstruction).
  • Backtest / simulation / notebook evidence (if applicable): Full suite green locally on both backends (8513 passed; two unrelated pre-existing local failures traced to a stale worktree Rust .so, resolved by rebuild — not code issues).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

…gregated_se

The per-(g,t) cluster-SE helper (_cluster_robust_se_from_per_gt_if) and
the general combined-IF assembly scattered per-cell influence functions
with np.add.at (unbuffered ufunc, 5-20x slower than fancy +=). Index
arrays are duplicate-free by construction at every producer (np.where
on disjoint masks — the invariant _combined_if_fast already relies on),
so the fancy scatter is bit-identical. Also removes the dead
_compute_aggregated_se (zero in-package callers; superseded by
_compute_aggregated_se_with_wif), updates the stale test_se_accuracy
docstring pointer, and refreshes the REGISTRY/dispatch-comment wording
about the general fallback path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected method: CallawaySantAnna group-time ATT influence-function aggregation and per-(g,t) cluster-robust SE assembly.
  • The np.add.at → fancy += rewrite preserves the documented mathematical contract where index arrays are producer-owned and duplicate-free.
  • Registry documentation was updated to describe the implementation choice, not a methodology deviation.
  • Dead _compute_aggregated_se removal appears safe: no in-package callers found; current aggregation routes through _compute_aggregated_se_with_wif.
  • I could not execute tests here because pytest is not installed in the environment.

Methodology

No findings.

The changed scatter sites still assemble the same per-index IF vectors before variance computation: diff_diff/staggered.py:L165-L181, diff_diff/staggered_aggregation.py:L305-L319, diff_diff/staggered_aggregation.py:L500-L520. The duplicate-free invariant is supported by producers using np.where over boolean masks in panel/vectorized/RCS paths: diff_diff/staggered.py:L1101-L1119, diff_diff/staggered.py:L1295-L1306, diff_diff/staggered.py:L1724-L1738, diff_diff/staggered.py:L3992-L4009.

The methodology registry documents the combined-IF contract and explicitly notes the fancy += scatter as bit-identical under that invariant: docs/methodology/REGISTRY.md:L599-L616.

Code Quality

No findings.

The deleted _compute_aggregated_se was not referenced by in-package code in my rg "_compute_aggregated_se\\(" . check, and aggregation continues through _compute_aggregated_se_with_wif: diff_diff/staggered_aggregation.py:L623-L701.

Performance

No findings.

The change removes np.add.at from the affected production scatter paths while keeping the existing fast-path/general-path structure.

Maintainability

No findings.

The comments and registry now align with the implementation and clearly state the duplicate-free index contract.

Tech Debt

No findings.

The PR removes the completed CS-scaling TODO rows and leaves the remaining related deferred allocation item tracked in TODO.md:L61.

Security

No findings.

The diff is limited to numerical aggregation code, docs, TODO/changelog, and a test docstring pointer; no secrets, file IO, network, or deserialization surfaces are introduced.

Documentation/Tests

No PR findings.

Registry and changelog documentation were updated. Test execution was not possible in this review environment: pytest and python -m pytest are unavailable.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 7, 2026
@igerber igerber merged commit 689c31c into main Jul 7, 2026
35 of 36 checks passed
@igerber igerber deleted the perf/cs-if-scatter-cleanup branch July 7, 2026 20:53
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