Skip to content

Commit 086e597

Browse files
Audit round 3: aggte() cryptic KeyError on non-CS input
Third-round audit focused on boundary conditions, data-flow edge cases, and cross-module API consistency. Only one genuine new issue surfaced; everything else (Mammen math, RCS IF normalisation, breakdown_m formula, ggdid ordering, save_to path handling, BJS Optional import) verified correct on close re-reading. Issue: `aggte(sa_result)` crashed with `KeyError: 'group'` deep inside `_weights_dynamic`, for the same reason as the round-2 `cs_report(sa_result)` bug — Sun-Abraham / BJS / dCDH event-study frames carry `relative_time` but no `group` / `time` columns. Round 2 fixed the cs_report entry point but aggte is also public API, so the same error can be hit directly: sa = sp.sun_abraham(...) sp.aggte(sa, type='dynamic') # <-- cryptic KeyError Fix: identical upfront guard in `aggte()` — require the CS detail schema {group, time, att, se, relative_time} and raise a clear ValueError pointing to honest_did() for non-CS event studies. Test: `tests/test_aggte.py::test_aggte_rejects_non_cs_result`. Full DiD suite: 170/170 (was 169). Items verified clean on this pass (kept notes in case a future refactor touches them): * Mammen bootstrap: E[V]=0, Var(V)=1 confirmed algebraically; IQR/iqr_norm equals σ for normal bootstrap draws. * SE scaling: psi = inf @ W.T, boot = V @ centered / n, SE from IQR gives the correct √(Var(ψ)/n). * RCS IF: 4-cell ratio estimator IF is proportional to 1{i ∈ cell}(Y_i − Ȳ_cell)/p_cell — correct for sample means. * breakdown_m: n_drift = max(e+1, 1) matches CS2021 smoothness bias bound M · (e + 1) for post-treatment e ≥ 0. * aggte-with-anticipation: `_get_gt_pairs` correctly skips cohorts lacking a valid pre-period; downstream `_cohort_weight_series` only uses cohorts actually present in detail, so the weights renormalise rather than double-count. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e5d68ec commit 086e597

3 files changed

Lines changed: 33 additions & 2 deletions

File tree

CONTRIBUTING.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ We welcome contributions to StatsPAI! This document provides guidelines for cont
1616

1717
1. **Fork the Repository**
1818
```bash
19-
git clone https://github.com/brycewang-stanford/pyEconometrics.git
20-
cd pyEconometrics
19+
git clone https://github.com/brycewang-stanford/StatsPAI.git
20+
cd StatsPAI
2121
```
2222

2323
2. **Set Up Development Environment**

src/statspai/did/aggte.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,17 @@ def aggte(
138138
"result has no ATT(g,t) detail to aggregate — was this produced "
139139
"by callaway_santanna()?"
140140
)
141+
# aggte needs the CS (g, t) grid; SA / BJS / dCDH event studies don't
142+
# carry it and would otherwise crash deep inside the weight builders.
143+
_required_cols = {'group', 'time', 'att', 'se', 'relative_time'}
144+
_have = set(detail.columns)
145+
if not _required_cols.issubset(_have):
146+
raise ValueError(
147+
"aggte() requires a Callaway–Sant'Anna result (detail frame "
148+
f"with columns {sorted(_required_cols)}). Got a result of "
149+
f"method {result.method!r} with columns {sorted(_have)}. For "
150+
"Sun–Abraham or BJS event studies use honest_did() directly."
151+
)
141152
if inf_matrix is None:
142153
# Analytic fallback still works but only gives pointwise intervals.
143154
bstrap = False

tests/test_aggte.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,3 +206,23 @@ def test_dynamic_overall_averages_post_event_only(cs_result):
206206
# Sanity: on this DGP the two differ enough that a silent regression
207207
# would be caught.
208208
assert not np.isclose(post_mean, all_mean, atol=1e-3)
209+
210+
211+
def test_aggte_rejects_non_cs_result():
212+
"""Passing SA / BJS output must raise a clear error instead of KeyError."""
213+
import numpy as np
214+
import statspai as sp
215+
216+
rng = np.random.default_rng(0)
217+
rows = []
218+
for u in range(60):
219+
g_val = [3, 5, 7, 0][u // 15]
220+
ui = rng.normal(scale=0.3)
221+
for t_ in range(1, 9):
222+
te = max(0, t_ - g_val + 1) * 0.5 if g_val > 0 else 0
223+
rows.append({'i': u, 't': t_, 'g': g_val,
224+
'y': ui + 0.2 * t_ + te + rng.normal()})
225+
df = pd.DataFrame(rows)
226+
sa = sp.sun_abraham(df, y='y', g='g', t='t', i='i')
227+
with pytest.raises(ValueError, match='Callaway'):
228+
aggte(sa, type='dynamic', n_boot=50, random_state=0)

0 commit comments

Comments
 (0)