Add TROP non-absorbing (on/off) treatment support (opt-in, local method)#592
Conversation
Overall Assessment✅ Looks good. No unmitigated P0/P1 findings. Executive Summary
MethodologyP3 informational, no action required Code QualityNo findings. The new parameter is validated, included in PerformanceNo blocking findings. The connected-component estimability guard adds per-treated-cell work, but it is documented as correctness-first and only materially active on support-degenerate cases: MaintainabilityNo findings. The bootstrap Tech DebtNo findings. I did not see a new untracked correctness deferral introduced by this PR. SecurityNo findings. No secrets or security-sensitive code paths observed in the diff. Documentation/TestsP2 P3 review note |
TROP now accepts general (on/off) assignment via TROP(non_absorbing=True), matching the source paper's scope (Athey, Imbens, Qu & Viviano 2025 §2.1 / Eq. 12 / Algorithm 2: "units moving into and out of treatment"). This removes a prior implementation over-restriction rather than adding a deviation; the default non_absorbing=False is unchanged and still rejects a non-monotonic indicator with a ValueError (now also pointing to the opt-in), guarding the common mistake of encoding absorbing treatment as an event-style spike. API / scope: - non_absorbing: bool = False on TROP (mirrors LPDiD); threaded through __init__ validation, get_params/set_params, and recorded on TROPResults / to_dict() / summary() so a persisted result keeps the assignment-scope context. - method='local' only; method='global' rejects non_absorbing=True (its post-hoc weighting + bootstrap assume a contiguous block). - One-time caveat UserWarning: validity relies on no-dynamic-effects, and the Theorem 5.1 triple-robustness guarantee is proven under block assignment only. Estimability guard (general correctness, applied to every local fit): - A treated cell (i,t) is estimable iff alpha_i + beta_t is identified by the two-way-FE control fit, i.e. the target unit node and period node lie in the same connected component of the bipartite graph of positively-weighted observed control cells (_treated_cell_is_estimable). Non-estimable cells (always-treated unit, fully-treated period, disconnected support; or unbalanced absorbing panels with entirely-missing unit/period controls) are NaN and excluded from the ATT, which is the mean over estimable cells -- matching the library-wide non-estimable->NaN convention. On balanced panels (and absorbing panels with an observed never-treated unit) the graph is one component, so the guard is a no-op (no behavior change), confirmed by the unchanged absorbing test suite. - The point fit and the bootstrap refit apply the identical predicate; the survey-weighted average is guarded against a zero total weight (NaN per the bootstrap contract, not ZeroDivisionError). Backends: - The Rust local LOOCV / point paths were already mask-driven and are unchanged (Rust/Python ATT parity regression-tested). The Rust per-cell bootstrap lacks the estimability guard, so the bootstrap is routed to the guarded Python path for non_absorbing fits, any unbalanced panel, or whenever the point fit trimmed a cell; balanced panels keep the Rust happy path. Docs: REGISTRY ## TROP (D-matrix semantics, validation, requirements checklist flipped from "out of scope" to supported, no-dynamic-effects + block-only inference + non-estimable-cell Notes, v3 consulted), athey-2025-review.md, METHODOLOGY_REVIEW.md, api/trop.rst, llms.txt, llms-full.txt, CHANGELOG, and docstrings. Also corrects the stale "only library estimator that handles non-absorbing treatment" claim across docs / AI-agent guides / dCDH docstrings (dCDH is the most general; LPDiD and TROP also support it under stronger assumptions). Tests: opt-in acceptance, default + event-style rejection, global-combo rejection, params round-trip, known-ATT recovery on a no-dynamic-effects toggling DGP, fully-toggling (no never-treated unit), unbalanced panels, always-treated unit (lambda_unit 0 and >0), fully-treated period, disconnected support, unbalanced-absorbing trimming, Rust/Python parity, Rust-bypass routing, and the survey Rao-Wu zero-weight guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
igerber
force-pushed
the
trop-non-absorbing
branch
from
5b5b6d9 to
4087c32
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good. No unmitigated P0/P1 findings. Executive Summary
MethodologyP3 informational, no action required Code QualityNo findings. The new constructor parameter is validated and included in PerformanceNo blocking findings. The connected-component estimability guard adds per-treated-cell work, but it is scoped to local fits and documented as a correctness guard; Rust remains available for clean local LOOCV and absorbing bootstrap paths. Locations: MaintainabilityNo findings. The support predicate is centralized in Tech DebtNo findings. I did not see a new untracked correctness deferral. The documented caveats are in SecurityNo findings. The diff does not introduce secrets, network calls, or security-sensitive behavior. Documentation/TestsP3 informational |
1 participant
Summary
TROPviaTROP(non_absorbing=True)(method='local'only), matching the source paper's general-assignment scope (Athey, Imbens, Qu & Viviano 2025, §2.1 / Eq. 12 / Algorithm 2). This removes a prior implementation over-restriction rather than adding a deviation; the defaultnon_absorbing=Falseis unchanged and still rejects a non-monotonic indicator (now pointing to the opt-in) to guard the common event-style-D mis-encoding.method='global'rejectsnon_absorbing=True(its post-hoc weighting + bootstrap assume a contiguous block). A one-timeUserWarningflags the no-dynamic-effects requirement and that Theorem 5.1's triple-robustness guarantee is proven under block assignment only.alpha_i + beta_tis identified by the two-way-FE control fit (target unit and period in the same connected component of the observed-control graph). Non-estimable cells (always-treated unit, fully-treated period, disconnected/missing control support) are materialized as NaN and excluded from the ATT — matching the library-wide non-estimable→NaN convention. On balanced / support-complete panels this is a no-op (no behavior change), confirmed by the unchanged absorbing test suite.non_absorbingonTROPResults/to_dict()/summary(). Rust local LOOCV / point paths are unchanged (parity-tested); the bootstrap routes to the guarded Python path for non-absorbing / unbalanced / trimmed fits (the Rust per-cell path lacks the guard), and the survey-weighted average is guarded against a zero total weight.Methodology references (required if estimator / math changes)
docs/methodology/REGISTRY.md## TROPanddocs/methodology/papers/athey-2025-review.md.**Note:**labels): (1) default rejects non-absorbing D (footgun guard); (2) non-absorbing is local-method only; (3) inference SEs use the bootstrap with a documented caveat that Theorem 5.1 is proven under block assignment only; (4) non-estimable cells (a degeneracy the paper does not cover, which assumes overlap-rich panels) are NaN-trimmed per the library non-estimable→NaN convention.Validation
tests/test_trop.py(TestDMatrixValidation: opt-in acceptance, global-combo rejection, params round-trip, results persistence, Rust/Python parity, unbalanced-panel Rust-bypass routing;TestTROPBootstrapFailureRateGuard: survey Rao-Wu zero-weight guard) andtests/test_methodology_trop.py(TestTROPDeviations: known-ATT recovery on a no-dynamic-effects toggling DGP, no-caveat-in-default, unbalanced × non-absorbing, always-treated unit atlambda_unit0 and >0, fully-treated period, disconnected support, unbalanced-absorbing trimming). Existing default-mode rejection tests retained.Security / privacy
Generated with Claude Code