Skip to content

Commit d35fe91

Browse files
committed
PR #454 R3 polish: warning string accuracy + boundary convention docs
R3 verdict was ✅ Looks good with two P3 informational items: 1. Warning string accuracy: the always-treated remap warning told users to "pass first_treat=0 or first_treat=np.inf explicitly to silence this warning," but `first_treat` is the column-name argument to `fit()`, not a value slot. Rewrote to "recode the affected rows' first_treat values to 0 or np.inf in your input data before fitting" to point users at the actual remediation surface. 2. Test class docstring: clarified that the implementation contract `first_treat <= min(time)` includes the `== min(time)` boundary case that the paper's strict `t_i < 1` shorthand excludes, and explained the pragmatic rationale (units treated at the first observable period have no untreated cell and cannot contribute to any 2x2 DD as a treated cohort). Aligns with merged registry note wording and removes the shorthand-vs-implementation ambiguity that the R3 reviewer flagged. Tests: 61 pass in test_methodology_bacon.py + test_bacon.py, 3 skipped (R parity).
1 parent 785c65f commit d35fe91

2 files changed

Lines changed: 23 additions & 11 deletions

File tree

diff_diff/bacon.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -575,9 +575,10 @@ def fit(
575575
f"(first_treat <= {min_period}, excluding sentinel values "
576576
f"0 and np.inf). Remapping to U bucket per Goodman-Bacon "
577577
f"(2021) footnote 11. The original first_treat column is "
578-
f"preserved; remapping happens in an internal column. Pass "
579-
f"first_treat=0 or first_treat=np.inf explicitly to silence "
580-
f"this warning.",
578+
f"preserved; remapping happens in an internal column. To "
579+
f"silence this warning, recode the affected rows' "
580+
f"first_treat values to 0 or np.inf in your input data "
581+
f"before fitting.",
581582
UserWarning,
582583
stacklevel=2,
583584
)

tests/test_methodology_bacon.py

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -487,14 +487,25 @@ def _panel_with_always_treated() -> pd.DataFrame:
487487

488488

489489
class TestBaconAlwaysTreatedRemap:
490-
"""Goodman-Bacon (2021) footnote 11: t_i < 1 units go in U.
491-
492-
bacon.py automatically remaps units whose ``first_treat <= min(time)``
493-
(excluding the never-treated sentinels ``0`` and ``np.inf``) via an
494-
internal column (``__bacon_first_treat_internal__``), preserving the
495-
user's original ``first_treat`` column unchanged. Detection uses
496-
ordered-time logic on the **time axis**, so event-time-encoded
497-
panels (``time ∈ [-2,..,3]``) are handled correctly.
490+
"""Goodman-Bacon (2021) footnote 11 with the library's first-period
491+
boundary convention.
492+
493+
The paper's footnote 11 says units treated before the first observable
494+
period (``t_i < 1`` under the paper's 1-indexed convention) belong in
495+
``U``. The library generalizes this to units whose
496+
``first_treat <= min(time)`` (i.e. includes ``first_treat == min(time)``,
497+
which the paper's strict ``<`` shorthand excludes). The library
498+
convention is pragmatic: units treated at the first observable period
499+
have no untreated cell within the panel and cannot contribute to any
500+
valid 2x2 DD as a treated cohort, so folding them into ``U`` mirrors
501+
the always-treated handling. The never-treated sentinels
502+
(``first_treat ∈ {0, np.inf}``) are excluded from the remap.
503+
504+
bacon.py applies the remap via an internal column
505+
(``__bacon_first_treat_internal__``), preserving the user's original
506+
``first_treat`` column unchanged. Detection uses ordered-time logic
507+
on the **time axis**, so event-time-encoded panels
508+
(``time ∈ [-2,..,3]``) are handled correctly.
498509
"""
499510

500511
def test_warn_emitted_on_remap(self) -> None:

0 commit comments

Comments
 (0)