perf(spillover): sparse cKDTree branch for staggered nearest-treated distances#645
Open
igerber wants to merge 1 commit into
Open
perf(spillover): sparse cKDTree branch for staggered nearest-treated distances#645igerber wants to merge 1 commit into
igerber wants to merge 1 commit into
Conversation
…distances The staggered cohort loop always built a dense (n_units, n_treated_by_onset) distance matrix per cohort. It now dispatches per cohort to the same cKDTree helper the static path uses (auto-activated past _CONLEY_SPARSE_N_THRESHOLD, built-in metrics only, cutoff_km = the outermost ring edge / _effective_d_bar). Within-cutoff distances are exact (the helper recomputes the true metric for in-range matches); beyond-cutoff units get inf — semantics-preserving because every staggered d_it consumer (ring membership, S_it, the far-away check, the event-study d_bar trigger) compares against thresholds at or below that cutoff. Helper- and fit-level equality tests pin the sparse arm against dense (atol 1e-12 end-to-end, trigger array identical). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Overall Assessment: ✅ Looks Good Executive Summary
Methodology
Code Quality
Performance
Maintainability
Tech Debt
Security
Documentation/Tests
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_compute_nearest_treated_distance_staggeredalways built a dense(n_units, n_treated_by_onset)distance matrix per cohort. It now dispatches per cohort to the same cKDTree helper the static path already uses (_compute_nearest_treated_distance_sparse) — auto-activated whenn_units > _CONLEY_SPARSE_N_THRESHOLD, built-in metrics only, withcutoff_km = _effective_d_bar(the outermost ring edge), mirroring the static call site exactly. Closes the Wave-B TODO row.inf, and every staggeredd_itconsumer — ring membership,S_it, the far-away-control check, and the event-studyd_bartrigger — compares against thresholds ≤ that cutoff, soinfvs large-finite is indistinguishable downstream (same argument as the documented static-path behavior).TestStaggeredSparseKDTreeBranch: helper-level equality (within-cutoff distances match dense at 1e-8; beyond-cutoff allinf; thed_bartrigger array identical incl. NaN pattern) and end-to-end fit identity (att/SE/per-ring effects at atol 1e-12) with the threshold monkeypatched to force the sparse arm.Methodology references (required if estimator / math changes)
Validation
tests/test_spillover.py::TestStaggeredSparseKDTreeBranch(2 tests); fulltests/test_spillover.pypasses (321).Security / privacy
Generated with Claude Code