Skip to content

Commit 049f04c

Browse files
igerberclaude
andcommitted
Address CI R13 P1: constant violation pattern is now a true level shift
R13 CI codex caught the next adjacent unit-mismatch — analogous to R12's linear/level fix but for `violation_type='constant'`. REGISTRY `## PreTrendsPower` documents the pattern as `δ_t = c` (per-period level shift), but `_get_violation_weights('constant')` still L2-normalized to `[1/√K, ..., 1/√K]`, so `δ_t = M/√K` not `δ_t = M`. The Step 4 γ-unit fix for linear and the R12 holistic `max_abs_pre_violation` fix uncovered this — once `mdv` is the documented magnitude of a per-period shift on linear, the constant pattern's silent `1/√K` re-scaling becomes visible and breaks the documented contract. **Holistic fix — constant/last_period skip L2 normalization** - `_get_violation_weights('constant')`: early return `np.ones(n_pre)`, no L2 norm. Now `δ_t = M` exactly per period — matching the REGISTRY/API documented contract. - `_get_violation_weights('last_period')`: also given an explicit early return for symmetry. The `[0, ..., 0, 1]` vector already had L2 norm 1, so this is a no-op numerically; the early return locks the contract uniformly across level-pattern violation types. - `power_at()` legacy reconstruction (fallback for old serialized results without `violation_weights`): unchanged — still L2-normalizes, preserving pre-PR-B numerical output for legacy serialized fits per the same backwards-compat policy applied to the linear-legacy and constant-legacy paths. - Docstring on `_get_violation_weights` rewritten to enumerate the per-type normalization convention explicitly: linear-with-times (γ-unit), linear-legacy (L2-norm), constant (level), last_period (level), custom (L2-norm). **End-to-end regression** in `test_methodology_pretrends.py`: - `test_constant_violation_pattern_is_level_shift`: real `SunAbraham`-fit results, asserts `violation_weights == [1, ..., 1]` (NOT L2-normalized → `||w||_2 = √K`), `max_abs_pre_violation == mdv` (level-scale and γ-scale coincide for constant), and `power_at(M) ≈ refit(M=M).power` at `atol=1e-4` so the level-shift contract holds end-to-end through `fit()` and `power_at()`. Codex specifically requested an end-to-end lock so future audits cannot drift between "per-period shift" and "normalized-direction magnitude". - `test_constant_weights` in `tests/test_pretrends.py` flipped: was pinning `np.linalg.norm(weights) == 1.0` (the OLD L2-normalized contract); now asserts unnormalized `[1, 1, 1, 1]` with `||w||_2 = 2` (√4). Docstring explains the contract change. **P3 fix — BR rendered label** `BusinessReport.full_report()` still labeled the `mdv_share_of_att` row as `MDV / |ATT|`, but R12 redefined the numerator as `max_abs_pre_violation`. Fix: rename the rendered label to "Max pre-period level deviation / |ATT|" and add an explicit row for `Max pre-period level deviation at MDV` above it so users can see both the raw `mdv` and the level-scale scalar. **Behavior change for users:** any caller passing `violation_type='constant'` will see a √K-factor change in the reported `mdv` and downstream `mdv_share_of_att`. The shift is documented in REGISTRY `## PreTrendsPower` (the pattern was `δ_t = c` all along — the IMPLEMENTATION is now what the docs have always said). 445 tests pass across pretrends + DR + BR + SA. 4 skipped (R-parity + 1 fixture skip). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent b053faa commit 049f04c

4 files changed

Lines changed: 222 additions & 162 deletions

File tree

diff_diff/business_report.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2467,11 +2467,18 @@ def _render_full_report(schema: Dict[str, Any]) -> str:
24672467
if tier:
24682468
lines.append(f"- Power tier: `{tier}`")
24692469
mdv = pt.get("mdv")
2470+
max_abs_pre = pt.get("max_abs_pre_violation")
24702471
ratio = pt.get("mdv_share_of_att")
24712472
if isinstance(mdv, (int, float)):
24722473
lines.append(f"- Minimum detectable violation (MDV): {mdv:.3g}")
2474+
if isinstance(max_abs_pre, (int, float)):
2475+
lines.append(f"- Max pre-period level deviation at MDV: {max_abs_pre:.3g}")
24732476
if isinstance(ratio, (int, float)):
2474-
lines.append(f"- MDV / |ATT|: {ratio:.2g}")
2477+
# PR-B R12: ratio is now max_abs_pre_violation / |ATT|, the
2478+
# level-scale comparable to ATT (not raw γ-unit mdv on linear
2479+
# fits). Label updated to match the numerator definition in
2480+
# REPORTING.md "Power-aware phrasing" Note.
2481+
lines.append(f"- Max pre-period level deviation / |ATT|: {ratio:.2g}")
24752482
else:
24762483
lines.append(f"- Pre-trends not computed: {pt.get('reason', 'unavailable')}")
24772484
lines.append("")

diff_diff/pretrends.py

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -890,11 +890,30 @@ def _get_violation_weights(
890890
Returns
891891
-------
892892
np.ndarray
893-
Violation weights. For ``violation_type='linear'`` with
894-
``relative_times`` provided: ``|t|`` directly, NOT L2-normalized
895-
(so ``M=γ`` directly under Roth's slope convention). For all
896-
other paths (constant, last_period, custom, or
897-
linear-without-relative_times): L2-normalized to unit norm.
893+
Violation weights, with per-violation-type normalization
894+
conventions chosen so the magnitude `M` matches what
895+
``REGISTRY.md`` documents for the pattern:
896+
897+
- ``'linear'`` with ``relative_times``: ``|t|`` directly,
898+
NOT L2-normalized (so ``δ_t = M * |t|`` and the reported
899+
MDV is in Roth's γ units). PR-B Step 4.
900+
- ``'linear'`` without ``relative_times`` (legacy): the
901+
count-based ``[n_pre-1, ..., 0]`` direction, L2-normalized
902+
to unit norm (preserves pre-PR-B shipped behavior).
903+
- ``'constant'``: ``[1, 1, ..., 1]`` directly, NOT
904+
normalized — ``δ_t = M`` per period (a true level shift,
905+
matching the documented ``δ_t = c`` convention). PR-B R13
906+
fix: pre-R13 normalization gave ``δ_t = M/√K``, a silent
907+
rescaling that the REGISTRY/API did not document.
908+
- ``'last_period'``: ``[0, ..., 0, 1]`` directly. Already
909+
unit-norm so the post-normalization output was identical;
910+
the unconditional early return locks the level-shift
911+
contract.
912+
- ``'custom'``: user-supplied ``violation_weights``,
913+
L2-normalized to unit norm (M is the magnitude along the
914+
user's direction; downstream
915+
``max_abs_pre_violation = M * max(|weights|)`` exposes
916+
the level-scale max under the MDV).
898917
"""
899918
if self.violation_type == "custom":
900919
assert self.violation_weights is not None
@@ -930,18 +949,33 @@ def _get_violation_weights(
930949
weights = np.arange(-n_pre + 1, 1, dtype=float)
931950
weights = -weights # Now [n-1, n-2, ..., 1, 0]
932951
elif self.violation_type == "constant":
933-
# Same violation in all periods
934-
weights = np.ones(n_pre)
952+
# δ_t = M for all pre-periods (level shift). Skip L2
953+
# normalization so M is exactly the per-period level shift
954+
# the REGISTRY documents (`δ_t = c`). Pre-PR-B (and the
955+
# pre-R13 PR-B state) divided by sqrt(K), making `δ_t =
956+
# M/sqrt(K)` and silently re-scaling reported MDV/power on
957+
# constant fits by sqrt(K). PR-B R13 fix: skip the norm
958+
# so the public contract matches the docs.
959+
return np.ones(n_pre, dtype=float)
935960
elif self.violation_type == "last_period":
936-
# Violation only in last pre-period (period -1)
937-
weights = np.zeros(n_pre)
961+
# Violation only in last pre-period (period -1). Unnormalized
962+
# `[0, ..., 0, 1]` already has L2 norm 1, so this path was
963+
# always equivalent to the post-normalization output; keep
964+
# the early return for symmetry with constant + linear-with-
965+
# relative_times so the level-shift contract is uniform
966+
# across all level-pattern violation types.
967+
weights = np.zeros(n_pre, dtype=float)
938968
weights[-1] = 1.0
969+
return weights
939970
else:
940971
raise ValueError(f"Unknown violation_type: {self.violation_type}")
941972

942973
# Normalize to unit norm (if not all zeros). The early-return
943-
# branch above for linear-with-relative_times intentionally skips
944-
# this normalization to preserve the γ-unit scale.
974+
# branches above for linear-with-relative_times, constant, and
975+
# last_period intentionally skip this normalization to preserve
976+
# the level-shift contract documented in REGISTRY.md
977+
# `## PreTrendsPower`. This block only fires for the linear-
978+
# legacy-fallback path and `violation_type='custom'`.
945979
norm = np.linalg.norm(weights)
946980
if norm > 0:
947981
weights = weights / norm

tests/test_methodology_pretrends.py

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,44 @@ def test_max_abs_pre_violation_uses_weight_scale_on_irregular_grid(self):
523523
res.max_abs_pre_violation > 4 * mdv
524524
), "max_abs_pre_violation must scale by max(|w|)=5, not collapse to mdv"
525525

526+
def test_constant_violation_pattern_is_level_shift(self, sa_results):
527+
"""``violation_type='constant'`` produces a per-period level shift,
528+
not an L2-normalized direction (PR-B R13 fix).
529+
530+
REGISTRY ``## PreTrendsPower`` documents constant as ``δ_t = c``.
531+
The implementation now returns unnormalized ``[1, 1, ..., 1]``
532+
weights so the contract holds at the public API surface:
533+
534+
- ``violation_weights == [1, 1, ..., 1]`` after fit (no L2 norm).
535+
- ``max_abs_pre_violation == mdv * 1 == mdv`` (level-scale and
536+
γ-scale coincide for the constant pattern).
537+
- ``power_at(M)`` evaluates the violation `δ_t = M` per period,
538+
not `δ_t = M/√K`.
539+
540+
Pre-PR-B-R13 the constant path was silently divided by √K,
541+
so a constant MDV of 0.5 was a per-period shift of 0.5/√K,
542+
not 0.5 as the docs claimed. Locks the level-shift contract
543+
end-to-end on a real fit.
544+
"""
545+
pt = PreTrendsPower(violation_type="constant", pretest_form="nis")
546+
result = pt.fit(sa_results)
547+
548+
n_pre = result.n_pre_periods
549+
# Weights are exactly [1, 1, ..., 1] — NOT L2-normalized.
550+
assert result.violation_weights is not None
551+
np.testing.assert_allclose(result.violation_weights, np.ones(n_pre))
552+
# L2 norm of weights is √K, not 1.
553+
assert np.isclose(np.linalg.norm(result.violation_weights), np.sqrt(n_pre))
554+
# Level-scale max coincides with raw mdv (max(|w|) = 1).
555+
assert np.isclose(result.max_abs_pre_violation, result.mdv)
556+
557+
# power_at(M) round-trip: under the level-shift contract,
558+
# power_at(M) for constant must equal power at `M=0.1` of a refit.
559+
# Loose atol because scipy MVN CDF and the centered helper take
560+
# slightly different paths with ~1e-6 sub-ULP roundoff.
561+
refit = pt.fit(sa_results, M=0.1)
562+
assert np.isclose(result.power_at(0.1), refit.power, atol=1e-4)
563+
526564
def test_is_informative_uses_level_scale_not_raw_gamma(self):
527565
"""``is_informative`` consumes ``max_abs_pre_violation`` (level scale)
528566
rather than raw ``mdv`` (slope scale) — locks the R12 fix on the

0 commit comments

Comments
 (0)