Audit round 2: 3 deeper semantic / UX bugs found and fixed

Second-pass audit of today's work — focused on algorithmic semantics
rather than mechanics.  Three genuine issues, all now fixed with
regression tests.

1. **aggte(type='dynamic').estimate averaged pre + post event times.**
   R `did::aggte(type='dynamic')` reports the overall ATT as a simple
   average of *post-treatment* event-study estimates (e ≥ 0).  Pre-
   treatment cells are placebos — averaging them into the headline
   number pollutes the causal summary with pre-trend signal.

   On the canonical demo DGP the bug produced overall = 0.7590
   (diluted by 6 pre-treatment rows with mean-zero placebos); the
   correct value is 1.4052 — off by nearly a factor of 2.

   Fixed in `_aggregate` computation of `w_overall`: for dynamic,
   restrict the averaging weights to cells where the label (event
   time) is ≥ 0.  Group and calendar aggregations are unchanged:
   by construction their weight matrices already pick only post-
   treatment (g, t) cells.

   Also switched the bootstrap-off fallback SE formula from
   `sqrt(mean(se²))` to `sqrt(sum(w² · se²))` so it respects the
   same weight vector and remains comparable with the bootstrap
   path.

   Docstring updated to explicitly state the four overall conventions.

2. **cs_report() crashed with KeyError: 'group' on non-CS results.**
   Before: passing a Sun-Abraham / BJS / dCDH result (which carry
   `relative_time` but not `group` / `time`) raised a cryptic
   pandas KeyError deep inside aggte's weight builder.
   Now: detects the missing CS schema up-front and raises a
   ValueError pointing the user to honest_did() for those result
   types.

3. **cs_report(pre_fitted_cs, estimator='reg', …) silently ignored
   the estimator override.**  A user reading the call site would
   reasonably believe the estimator was applied; in fact the pre-
   fit's settings were used verbatim.  Now emits a UserWarning that
   lists every shadowed argument and tells the caller to pass raw
   data instead if they want to re-estimate.

Regression tests (+4 new cases):
 - `test_dynamic_overall_averages_post_event_only`
 - `test_cs_report_rejects_non_cs_result`
 - `test_cs_report_warns_on_shadowed_args`
 - `test_cs_report_pre_fitted_without_shadowed_args_is_silent`

Full DiD suite: 169/169 (was 165).

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

Author: brycewang-stanford

src/statspai/did/aggte.py

@@ -95,9 +95,16 @@ def aggte(
     Returns
     -------
     CausalResult
-        ``.estimate`` / ``.se`` hold the overall aggregated ATT (for
-        ``simple``: the single number; for the others: the un-weighted
-        average over the reported cells, matching R's ``did::aggte``).
+        ``.estimate`` / ``.se`` hold the overall aggregated ATT,
+        matching R's ``did::aggte`` print convention:
+
+        - ``'simple'``   — the single cohort-share-weighted overall ATT
+        - ``'dynamic'``  — simple average of *post-treatment* event times
+          (e ≥ 0); pre-treatment cells are placebos and excluded
+        - ``'group'``    — simple average of the per-cohort θ(g) estimates
+        - ``'calendar'`` — simple average of the per-calendar-time θ(t)
+          estimates
+
         ``.detail`` is a tidy frame with one row per aggregation cell and
         both pointwise and (if requested) uniform bands.
 
@@ -195,18 +202,39 @@ def aggte(
         out['cband_upper'] = est_cells + crit_unif * se_cells
         out['crit_val_uniform'] = crit_unif
 
-    # "Overall" summary: simple mean of cells (matches R's aggte print).
+    # "Overall" summary — matches R's did::aggte print() convention:
+    #   simple   : the single cohort-share-weighted overall ATT
+    #   dynamic  : simple average of POST-treatment event times only
+    #              (pre-treatment cells are placebos, not part of the
+    #              overall causal summary)
+    #   group    : simple average across cohorts (all post-treatment by
+    #              construction of the θ(g) weights)
+    #   calendar : simple average across calendar times (all post-treatment
+    #              by construction)
     if type == 'simple':
         overall_est = float(est_cells[0])
         overall_se = float(se_cells[0])
     else:
-        w_overall = np.full(W.shape[0], 1.0 / W.shape[0])
+        if type == 'dynamic':
+            post_mask_agg = np.asarray(labels, dtype=float) >= 0
+            if not post_mask_agg.any():
+                # No post-treatment cells survived the min_e / max_e filter
+                # — fall back to the legacy "mean of all reported cells"
+                # behaviour so the caller still gets a number.
+                post_mask_agg = np.ones(W.shape[0], dtype=bool)
+            idx = np.where(post_mask_agg)[0]
+            w_overall = np.zeros(W.shape[0])
+            w_overall[idx] = 1.0 / idx.size
+        else:
+            w_overall = np.full(W.shape[0], 1.0 / W.shape[0])
         overall_est = float(w_overall @ est_cells)
         if bstrap and inf_matrix is not None:
             agg_inf = (w_overall @ W) @ inf_matrix.T
             overall_se = float(np.sqrt(np.mean(agg_inf ** 2) / n_units))
         else:
-            overall_se = float(np.sqrt(np.mean(se_cells ** 2)))
+            overall_se = float(
+                np.sqrt(np.sum((w_overall ** 2) * se_cells ** 2))
+            )
 
     overall_z = overall_est / overall_se if overall_se > 0 else 0.0
     overall_pval = float(2 * (1 - stats.norm.cdf(abs(overall_z))))

src/statspai/did/report.py

@@ -680,6 +680,45 @@ def cs_report(
     """
     if isinstance(data_or_result, CausalResult):
         cs = data_or_result
+        # Warn if the caller also supplied estimation-time arguments that
+        # would normally run a fresh fit — they are silently ignored when
+        # a pre-fitted result is passed, which is easy to misread as
+        # "the report re-estimated under my new settings".
+        import warnings as _warnings
+        _shadowed = []
+        if y is not None: _shadowed.append(f'y={y!r}')
+        if g is not None: _shadowed.append(f'g={g!r}')
+        if t is not None: _shadowed.append(f't={t!r}')
+        if i is not None: _shadowed.append(f'i={i!r}')
+        if x is not None: _shadowed.append(f'x={x!r}')
+        if estimator != 'dr': _shadowed.append(f'estimator={estimator!r}')
+        if control_group != 'nevertreated':
+            _shadowed.append(f'control_group={control_group!r}')
+        if anticipation != 0: _shadowed.append(f'anticipation={anticipation}')
+        if _shadowed:
+            _warnings.warn(
+                "cs_report() received a pre-fitted CausalResult together "
+                "with estimation-time arguments ("
+                + ", ".join(_shadowed)
+                + ") — those arguments are ignored and the pre-fitted "
+                "result is used as-is.  Pass raw data instead if you "
+                "want to re-estimate under the new settings.",
+                stacklevel=2,
+            )
+        # aggte requires (group, time) in detail — that is the CS schema.
+        # SA / BJS / dCDH use a different layout (relative_time only), so
+        # passing one of those here would later fail deep inside aggte with
+        # a cryptic KeyError.  Fail fast with an actionable message.
+        required = {'group', 'time', 'att', 'se', 'relative_time'}
+        have = set(cs.detail.columns) if cs.detail is not None else set()
+        if not required.issubset(have):
+            raise ValueError(
+                "cs_report() requires a Callaway–Sant'Anna result (with "
+                "'group' and 'time' columns in its detail frame).  "
+                f"Got a result of method {cs.method!r} with columns "
+                f"{sorted(have)}.  For Sun–Abraham or BJS results use "
+                "honest_did() directly on the event study."
+            )
     else:
         if not all([y, g, t, i]):
             raise ValueError(

tests/test_aggte.py

@@ -186,3 +186,23 @@ def test_ggdid_all_aggregation_types(cs_result):
         if typ != 'simple':
             labels = [t.get_text() for t in ax.get_legend().get_texts()]
             assert any('Uniform' in lab for lab in labels)
+
+
+def test_dynamic_overall_averages_post_event_only(cs_result):
+    """`aggte(type='dynamic').estimate` must be the mean of e≥0 cells only.
+
+    Previously the overall averaged *all* reported cells — including
+    pre-treatment event times — which polluted the headline number
+    with placebo signal.  Matches R `did::aggte` convention.
+    """
+    import numpy as np
+    es = aggte(cs_result, type='dynamic', n_boot=100, random_state=0)
+    post_mean = es.detail.loc[
+        es.detail['relative_time'] >= 0, 'att'
+    ].mean()
+    all_mean = es.detail['att'].mean()
+    # Overall must equal post-event mean, NOT all-cells mean.
+    assert es.estimate == pytest.approx(post_mean, abs=1e-10)
+    # Sanity: on this DGP the two differ enough that a silent regression
+    # would be caught.
+    assert not np.isclose(post_mean, all_mean, atol=1e-3)

tests/test_cs_report.py

@@ -283,3 +283,45 @@ def test_save_to_expands_tilde(tmp_path, monkeypatch):
     # If ~ was not expanded, we'd see a literal "~" subdirectory under CWD.
     assert (tmp_path / "cs_study" / "rpt.md").exists()
     assert (tmp_path / "cs_study" / "rpt.txt").exists()
+
+
+# --------------------------------------------------------------------------- #
+# Round-2 audit regressions                                                   #
+# --------------------------------------------------------------------------- #
+
+def test_cs_report_rejects_non_cs_result():
+    """Passing a Sun-Abraham / BJS result must raise a clear ValueError,
+    not a cryptic KeyError deep inside aggte."""
+    import statspai as sp
+    df = _staggered_panel(seed=19)
+    sa = sp.sun_abraham(df, y='y', g='g', t='t', i='i')
+    with pytest.raises(ValueError, match="Callaway"):
+        cs_report(sa, n_boot=50, random_state=0, verbose=False)
+
+
+def test_cs_report_warns_on_shadowed_args():
+    """Passing a pre-fitted result + estimation-time args must warn.
+
+    Otherwise users would silently see the pre-fit's settings rather
+    than the ones they explicitly passed."""
+    import warnings
+    df = _staggered_panel(seed=21)
+    cs = callaway_santanna(df, y='y', g='g', t='t', i='i', estimator='dr')
+    with warnings.catch_warnings(record=True) as w_list:
+        warnings.simplefilter("always")
+        cs_report(cs, estimator='reg',
+                  n_boot=30, random_state=0, verbose=False)
+    assert any("estimator='reg'" in str(wi.message) for wi in w_list)
+
+
+def test_cs_report_pre_fitted_without_shadowed_args_is_silent():
+    """No warning when user correctly passes only bootstrap/report args."""
+    import warnings
+    df = _staggered_panel(seed=23)
+    cs = callaway_santanna(df, y='y', g='g', t='t', i='i')
+    with warnings.catch_warnings(record=True) as w_list:
+        warnings.simplefilter("always")
+        cs_report(cs, n_boot=30, random_state=0, verbose=False)
+    # The only warning we tolerate here would be unrelated (none expected).
+    msgs = [str(wi.message) for wi in w_list]
+    assert not any("estimation-time" in m for m in msgs), msgs