Skip to content

Commit 853e523

Browse files
igerberclaude
andcommitted
Address CI R14 (P2 + P3) on PreTrendsPower PR-B
R14 verdict ✅ Looks good with one P2 BR-schema-gap + a P3 fan-out of stale "MDV vs ATT" prose that the R12 level-scale fix needed to sweep through but missed. **P2 — `_lift_pre_trends` dropped `max_abs_pre_violation`** R12 added `max_abs_pre_violation` to the `DiagnosticReport` schema and wired the `BusinessReport.full_report()` renderer to print it, but the BR schema-lift helper at `business_report.py:884` did NOT carry the field across the DR → BR boundary. Net effect: `BR.to_dict()["pre_trends"]` was missing the field AND `full_report()`'s `pt.get("max_abs_pre_violation")` returned None, so the new "Max pre-period level deviation at MDV" line never actually rendered. Fix: add `"max_abs_pre_violation": pp.get("max_abs_pre_violation")` to the `_lift_pre_trends` return dict. New BR end-to-end regression asserts both `BR.to_dict()["pre_trends"]["max_abs_pre_violation"]` is populated AND `full_report()` contains the rendered line. **P3 — Stale "MDV / |ATT|" prose in 4 surfaces** R12 moved the tier numerator from raw `mdv` to `max_abs_pre_violation` but several user-facing prose surfaces still said the comparison was between "MDV" and "estimated effect" — wording lag, not a behavioral bug. 1. `business_report.py:2167` "the test is well-powered" summary sentence: reworded to say "the max pre-period level deviation at the MDV is small relative to the estimated effect" rather than the bare "minimum-detectable violation is small". 2. `diagnostic_report.py:3284` DR "no_detected_violation / well_powered" sentence: same swap from "MDV is a small share of the estimated effect" to "the max pre-period level deviation at the MDV is a small share". 3. `PreTrendsPowerResults.violation_weights` docstring: reworded to enumerate per-violation_type normalization explicitly (linear with-times γ-unit; linear legacy L2-norm; constant unnormalized level-shift; last_period level; custom L2-norm). 4. `PreTrendsPowerResults.max_abs_pre_violation` property docstring (the non-linear-types paragraph): updated to reflect the R13 constant-level-shift change (`mdv * 1 = mdv` rather than the old `mdv / √K`). Plus the autosummary RST adds the new `~PreTrendsPowerResults.max_abs_pre_violation` property entry so the published API page lists it. R-parity P3 deferred to PR-C per the existing TODO row. 591 tests pass; no code-path regressions. The new BR regression catches the lift-boundary bug if it ever regresses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 049f04c commit 853e523

5 files changed

Lines changed: 57 additions & 15 deletions

File tree

diff_diff/business_report.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -924,6 +924,13 @@ def _lift_pre_trends(dr: Optional[Dict[str, Any]]) -> Dict[str, Any]:
924924
"power_reason": pp.get("reason"),
925925
"power_tier": pp.get("tier"),
926926
"mdv": pp.get("mdv"),
927+
# Level-scale max pre-period violation under the MDV
928+
# (PR-B R12: `mdv * max(|violation_weights|)`). Carried alongside
929+
# the raw `mdv` so BR schema consumers and the full-report
930+
# renderer can show both quantities. Pre-R14 this was silently
931+
# dropped at the BR lift boundary so the new renderer line never
932+
# fired even though DR emitted the value.
933+
"max_abs_pre_violation": pp.get("max_abs_pre_violation"),
927934
"mdv_share_of_att": pp.get("mdv_share_of_att"),
928935
# Carry the covariance-source annotation through so BR can hedge the
929936
# power-tier phrasing when compute_pretrends_power silently used a
@@ -2158,8 +2165,9 @@ def _render_summary(schema: Dict[str, Any]) -> str:
21582165
if tier == "well_powered":
21592166
sentences.append(
21602167
f"{subject} are consistent with parallel trends, and "
2161-
"the test is well-powered (the minimum-detectable "
2162-
"violation is small relative to the estimated effect)."
2168+
"the test is well-powered (the max pre-period level "
2169+
"deviation at the MDV is small relative to the "
2170+
"estimated effect)."
21632171
)
21642172
elif tier == "moderately_powered":
21652173
sentences.append(

diff_diff/diagnostic_report.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3281,9 +3281,10 @@ def _render_overall_interpretation(schema: Dict[str, Any], labels: Dict[str, str
32813281
if tier == "well_powered":
32823282
sentences.append(
32833283
f"{subject} are consistent with parallel trends"
3284-
f"{jp_str} and the test is well-powered (MDV is a small "
3285-
"share of the estimated effect), so a material pre-trend "
3286-
"would likely have been detected."
3284+
f"{jp_str} and the test is well-powered (the max pre-period "
3285+
"level deviation at the MDV is a small share of the estimated "
3286+
"effect), so a material pre-trend would likely have been "
3287+
"detected."
32873288
)
32883289
elif tier == "moderately_powered":
32893290
sentences.append(

diff_diff/pretrends.py

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -288,12 +288,19 @@ class PreTrendsPowerResults:
288288
alternative ``M * weights``. NIS-only; NaN for Wald fits.
289289
violation_weights : np.ndarray, optional
290290
The violation-direction vector used at fit time. Populated for all
291-
violation types on fresh fits. Normalization depends on the type:
292-
``constant`` / ``last_period`` / ``custom`` (or ``linear`` without
293-
``relative_times``) are stored L2-normalized; ``linear`` threaded
294-
with ``relative_times`` (the post-PR-B Step 4 γ-unit path)
295-
intentionally persists the unnormalized ``|t|`` direction so that
296-
``δ_pre = M · |t|`` and the reported MDV equals Roth's γ exactly.
291+
violation types on fresh fits. Normalization depends on the type
292+
so that ``M`` always matches the documented per-pattern contract:
293+
294+
- ``linear`` threaded with ``relative_times`` (post PR-B Step 4):
295+
``|t|`` directly, NOT L2-normalized, so ``δ_t = M·|t|`` and the
296+
reported MDV equals Roth's γ exactly.
297+
- ``linear`` without ``relative_times`` (legacy):
298+
``[n_pre-1, ..., 0]`` L2-normalized.
299+
- ``constant`` (post PR-B R13): ``[1, ..., 1]`` directly, NOT
300+
L2-normalized, so ``δ_t = M`` is a true per-period level shift.
301+
- ``last_period``: ``[0, ..., 0, 1]`` (already unit-norm).
302+
- ``custom``: user vector L2-normalized to unit norm.
303+
297304
Old serialized results may have ``None`` here; ``power_at()``
298305
falls back to reconstruction in that case (with the PR-A
299306
``NotImplementedError`` guard retained only for
@@ -367,10 +374,14 @@ def max_abs_pre_violation(self) -> float:
367374
alone cannot be compared to level effects without applying
368375
the weight scale.
369376
370-
For non-linear violation types: constant weights ``[1/√K, ...,
371-
1/√K]`` yield ``max_abs_pre_violation = mdv / √K``;
372-
last_period ``[0, ..., 0, 1]`` yields ``max_abs_pre_violation
373-
= mdv``; custom uses the user-supplied weight vector.
377+
For non-linear violation types under the PR-B R13 level-shift
378+
convention: constant weights ``[1, ..., 1]`` (unnormalized)
379+
yield ``max_abs_pre_violation = mdv * 1 = mdv`` — raw ``mdv``
380+
IS the per-period level shift, so level- and γ-scales coincide.
381+
Last_period ``[0, ..., 0, 1]`` yields ``max_abs_pre_violation
382+
= mdv`` for the same reason. Custom uses the L2-normalized
383+
user-supplied weight vector, so ``max_abs_pre_violation``
384+
depends on the user's direction.
374385
375386
Backwards-compat: legacy serialized results without
376387
``violation_weights`` (pre-PR-B) fall back to the raw ``mdv``

docs/api/_autosummary/diff_diff.PreTrendsPowerResults.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
.. autosummary::
2727

2828
~PreTrendsPowerResults.is_informative
29+
~PreTrendsPowerResults.max_abs_pre_violation
2930
~PreTrendsPowerResults.original_results
3031
~PreTrendsPowerResults.power_adequate
3132
~PreTrendsPowerResults.power

tests/test_business_report.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2506,6 +2506,27 @@ def test_full_vcov_path_no_downgrade_on_real_cs_fit(self, cs_fit):
25062506
assert "moderately informative" not in full.lower()
25072507
assert "moderately-informative" not in full.lower()
25082508

2509+
# PR-B R14 P2: max_abs_pre_violation must round-trip through the
2510+
# BR schema lift AND render in full_report(). Pre-R14 the field
2511+
# was emitted by DR, the renderer printed it, but the BR lift
2512+
# boundary at `_lift_pre_trends` silently dropped it — so the
2513+
# rendered line never fired even though the renderer had the
2514+
# branch.
2515+
br_schema = br.to_dict()
2516+
pt_block = br_schema.get("pre_trends", {})
2517+
assert "max_abs_pre_violation" in pt_block, (
2518+
"BR.to_dict()['pre_trends'] must surface max_abs_pre_violation "
2519+
"post-PR-B R14 — _lift_pre_trends regression"
2520+
)
2521+
assert pt_block["max_abs_pre_violation"] is not None
2522+
assert np.isclose(pt_block["max_abs_pre_violation"], 0.375, atol=0.05)
2523+
# full_report() must render the new "Max pre-period level
2524+
# deviation at MDV" line.
2525+
assert "Max pre-period level deviation at MDV:" in full, (
2526+
"BR.full_report() must render the max_abs_pre_violation line "
2527+
"(renderer wired in R12; lift boundary fixed in R14)"
2528+
)
2529+
25092530

25102531
class TestCSNotYetTreatedControlGroupSemantics:
25112532
"""Round-13 P1 regression: ``BusinessReport`` must not relabel

0 commit comments

Comments
 (0)