Skip to content

Commit 86c4a5a

Browse files
igerberclaude
andcommitted
Address thirty-eighth round of CI review findings on PR #318
P2 methodology (StaggeredTripleDiff fixed-control prose incomplete). Round-37 moved StaggeredTripleDiff's fixed ``control_group="never_ treated"`` schema to ``n_never_enabled`` (REGISTRY.md line 1730 names the never-enabled cohort as the valid fixed comparison) and cleared the composite ``n_control_units`` total from ``n_control``. The renderers, however, only surface ``n_never_enabled`` inside the ``is_dynamic_control`` branch — so the fixed ``never_treated`` path fell through to the generic ``Sample: N observations.`` sentence and the full report omitted the fixed comparison cohort entirely. Added dedicated fixed-never-enabled branches to both renderers: * ``_render_summary`` emits ``Sample: N observations (N_t treated, N_ne never-enabled).`` when the estimator is ``StaggeredTripleDiffResults``, the dynamic branch is not active, and ``n_never_enabled > 0``; * ``_render_full_report`` emits a dedicated bullet ``- Never-enabled units (fixed comparison cohort): N_ne`` under the same condition. P3 coverage. Round-37 regression only asserted absence of the wrong ``500 control`` wording; it did not positively assert the valid never-enabled comparison cohort appeared in rendered prose, which is why the P2 above slipped through. Regressions extended: * ``test_never_treated_mode_summary_renders_never_enabled_count`` asserts ``300 never-enabled`` appears in ``summary()`` AND the generic fallback ``Sample: 800 observations.`` does not fire; * new ``test_never_treated_mode_full_report_renders_never_enabled_count`` asserts the sample section of ``full_report()`` names ``never-enabled`` and the ``300`` count while omitting any bare ``- Control: 500`` line. 278 BR / DR / practitioner tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8838303 commit 86c4a5a

2 files changed

Lines changed: 63 additions & 7 deletions

File tree

diff_diff/business_report.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1931,6 +1931,25 @@ def _render_summary(schema: Dict[str, Any]) -> str:
19311931
"dynamic not-yet-treated comparison group (the control set "
19321932
f"varies by cohort and period){subset_clause}."
19331933
)
1934+
elif (
1935+
estimator == "StaggeredTripleDiffResults"
1936+
and isinstance(n_t, int)
1937+
and isinstance(n_ne, int)
1938+
and n_ne > 0
1939+
):
1940+
# Round-38 P2 CI review on PR #318: StaggeredTripleDiff
1941+
# under fixed ``control_group="never_treated"`` had the
1942+
# schema moved to ``n_never_enabled`` (round-37) but the
1943+
# renderers fell through to the generic
1944+
# ``Sample: N observations.`` sentence because the
1945+
# ``is_dynamic_control`` branch didn't fire. REGISTRY.md
1946+
# §StaggeredTripleDifference line 1730 names the
1947+
# never-enabled cohort as the valid fixed comparison on
1948+
# this path; the prose must say so.
1949+
sentences.append(
1950+
f"Sample: {n_obs:,} observations ({n_t:,} treated, "
1951+
f"{n_ne:,} never-enabled)."
1952+
)
19341953
else:
19351954
sentences.append(f"Sample: {n_obs:,} observations.")
19361955
survey = sample.get("survey")
@@ -2106,6 +2125,23 @@ def _render_full_report(schema: Dict[str, Any]) -> str:
21062125
)
21072126
if isinstance(sample.get("n_control"), int):
21082127
lines.append(f"- Control: {sample['n_control']:,}")
2128+
elif (
2129+
estimator_name == "StaggeredTripleDiffResults"
2130+
and isinstance(sample.get("n_never_enabled"), int)
2131+
and sample["n_never_enabled"] > 0
2132+
and not sample.get("dynamic_control")
2133+
):
2134+
# Round-38 P2 CI review on PR #318: fixed
2135+
# ``control_group="never_treated"`` on StaggeredTripleDiff
2136+
# clears ``n_control`` (composite total) and populates
2137+
# ``n_never_enabled`` (the valid fixed comparison cohort per
2138+
# REGISTRY.md line 1730). The full report must render that
2139+
# fixed count — the dynamic-control branch below would not
2140+
# fire on this path.
2141+
lines.append(
2142+
f"- Never-enabled units (fixed comparison cohort): "
2143+
f"{sample['n_never_enabled']:,}"
2144+
)
21092145
elif sample.get("dynamic_control"):
21102146
if isinstance(sample.get("n_never_enabled"), int) and sample["n_never_enabled"] > 0:
21112147
lines.append(

tests/test_business_report.py

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1333,18 +1333,38 @@ def test_never_treated_mode_surfaces_never_enabled_not_composite_total(self):
13331333
)
13341334
assert sample["n_never_enabled"] == 300
13351335

1336-
def test_never_treated_mode_summary_does_not_narrate_composite_as_control(self):
1336+
def test_never_treated_mode_summary_renders_never_enabled_count(self):
1337+
"""Round-38 P3 strengthened regression: the summary must
1338+
POSITIVELY surface the valid fixed comparison cohort
1339+
(``300 never-enabled``), not merely avoid the wrong
1340+
``500 control`` phrasing.
1341+
"""
1342+
import re
1343+
13371344
summary = BusinessReport(
13381345
self._stub("never_treated"), auto_diagnostics=False
13391346
).summary()
1340-
# The composite total must not appear as "500 control" in prose.
1341-
import re
1342-
1343-
assert not re.search(r"\b500\s+control", summary), (
1344-
f"BR summary must not narrate the composite n_control_units "
1345-
f"total as 'control' on StaggeredTripleDiff(control_group="
1347+
# Old wrong phrasing absent.
1348+
assert not re.search(r"\b500\s+control", summary), summary
1349+
# New fixed cohort present.
1350+
assert "300 never-enabled" in summary, (
1351+
f"BR summary must render the valid fixed never-enabled "
1352+
f"comparison cohort on StaggeredTripleDiff(control_group="
13461353
f"'never_treated'); got: {summary!r}"
13471354
)
1355+
# And the generic no-comparison fallback must not fire.
1356+
assert "Sample: 800 observations." not in summary
1357+
1358+
def test_never_treated_mode_full_report_renders_never_enabled_count(self):
1359+
md = BusinessReport(
1360+
self._stub("never_treated"), auto_diagnostics=False
1361+
).full_report()
1362+
sample_section = md.split("## Sample", 1)[1].split("\n## ", 1)[0]
1363+
assert "never-enabled" in sample_section.lower()
1364+
assert "300" in sample_section
1365+
# No bare "- Control: 500" line (composite total) should appear
1366+
# on this path.
1367+
assert "- Control: 500" not in sample_section
13481368

13491369

13501370
class TestBRHeadlineOmitsBrokenCIOnUndefinedInference:

0 commit comments

Comments
 (0)