Rename TROP method="twostep" to method="local"

[igerber]

Summary

• Rename TROP method="twostep" to method="local", forming a natural local/global pair
• Add FutureWarning deprecation for "twostep" → "local" (mirrors existing "joint" → "global"); both removed in v3.0
• Rename 8 internal _joint_* methods to _global_* for consistency
• Rename Rust exports loocv_grid_search_joint → loocv_grid_search_global and bootstrap_trop_variance_joint → bootstrap_trop_variance_global
• Update all tests, documentation, and CHANGELOG

Methodology references (required if estimator / math changes)

• Method name(s): TROP (Triply Robust Panel)
• Paper / source link(s): Athey, Imbens, Qu & Viviano (2025) https://arxiv.org/abs/2508.21536
• Any intentional deviations from the source (and why): None — purely a naming change with no behavioral modifications

Validation

• Tests added/updated: tests/test_trop.py (class/method renames, 2 new deprecation tests for "twostep"), tests/test_rust_backend.py (class/method renames, import/mock target updates)
• Full non-slow suite: 1809 passed, 0 failed
• Rust backend compiles and renamed exports verified
• Manual verification: TROP() defaults to "local", TROP(method="twostep") emits FutureWarning and normalizes to "local", TROP(method="joint") emits FutureWarning and normalizes to "global"

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

Overall Assessment

✅ Looks good

No unmitigated P0 or P1 findings in the diff. The twostep→local and joint→global change appears to be nomenclature-only: the paper still defines TROP through the same multi-treated-unit estimator, LOOCV tuning criterion, and bootstrap inference flow, and this repo now documents the rename explicitly in the Methodology Registry rather than silently changing the estimator. (ar5iv.org)

Executive Summary

• The rename is methodologically safe: it is documented in the registry and does not appear to change estimator logic, weighting, variance estimation, or identification checks.
• Python alias handling looks complete: deprecated names are normalized in __init__ and set_params(), get_params() returns canonical names, and fit() dispatches only on canonical self.method.
• Rust export renames were propagated through Python imports and Rust-backend tests.
• I found two P3 informational inconsistencies only: stale joint wording in Rust comments and a stale contributor-facing description of the global estimator in a renamed test class docstring.
• I could not execute the test suite in this environment because pytest is not installed.

Methodology

• P3 Informational. Impact: none. The rename from twostep/joint to local/global is a documented library-side deviation, not a methodology defect. The canonical names and deprecated aliases are implemented at diff_diff/trop.py:L66, diff_diff/trop.py:L142, diff_diff/trop.py:L2655, and docs/methodology/REGISTRY.md:L1486. The source paper still describes the same Algorithm 2 estimator, Equation 5 LOOCV criterion, and Algorithm 3 bootstrap flow. Concrete fix: none required. (ar5iv.org)

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• P3 Informational. Impact: the changed Rust file still documents the renamed global helpers as “joint” code paths, which leaves an avoidable translation layer between the public API and backend internals. See rust/src/trop.rs:L1087, rust/src/trop.rs:L1425, rust/src/trop.rs:L1505, and rust/src/trop.rs:L1633. Concrete fix: update those Rust doc comments now; optionally rename the remaining internal *_joint_* helpers in a later cleanup if full cross-language consistency is the goal.

Tech Debt

• No findings requiring a new TODO.md entry.

Security

• No findings.

Documentation/Tests

• P3 Informational. Impact: the renamed TestTROPGlobalMethod class still describes the global estimator using the older “single scalar tau via weighted least squares” mental model, while the implementation and registry describe global TROP as a single control-only fit with post-hoc per-observation residual effects averaged to ATT. See tests/test_trop.py:L2722, diff_diff/trop.py:L1005, and docs/methodology/REGISTRY.md:L1396. Concrete fix: rewrite that class docstring to match the current global-method semantics.
• No functional test-coverage gaps stood out in the diff itself, but I could not run tests here because pytest is unavailable in the environment.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 1e6bcd195169ee531f3888570142b8e48b1f65de

---

Overall Assessment

✅ Looks good

Executive Summary

• Re-review outcome: the PR remains a nomenclature change for TROP rather than a behavioral change. The affected methods are method="local" (formerly twostep) and method="global" (formerly joint).
• Cross-checking the code, registry, and cited method description, I did not find an unmitigated P0/P1 issue in estimator logic, weighting, bootstrap SE, inference, or assumption enforcement.
• The rename itself is documented in the Methodology Registry, so twostep→local and joint→global are not methodology defects.
• Previous informational findings from the last AI review are resolved: the Rust comments and the global-method test-class docstring now match the residual-based global implementation.
• One P2 methodology-documentation issue remains: the registry’s generic optimization summary still describes the global path as if it jointly estimates a scalar τ·D, which conflicts with the actual global implementation and the later registry section.
• Verification was static only: pytest is not installed here, and importing diff_diff also fails because numpy is missing.

Methodology

Affected methods: TROP method="local" and method="global".

• Severity: P2
  Impact: docs/methodology/REGISTRY.md:1278 and docs/methodology/REGISTRY.md:1284 now say the global path solves for (μ, α, β, τ) and uses τ·D in the residual. That conflicts with the actual Python and Rust implementations, which solve only for (μ, α, β, L) and extract τ_it post hoc at diff_diff/trop.py:667, diff_diff/trop.py:689, and rust/src/trop.rs:1199. The later global-method registry section already documents the correct residual-based formulation at docs/methodology/REGISTRY.md:1383 and docs/methodology/REGISTRY.md:1396.
  Concrete fix: Rewrite the generic optimization summary to either describe only the local Algorithm 2 solver, or explicitly state that the global path fits (μ, α, β, L) on control observations and recovers τ_it afterward.

• Severity: P3
  Impact: none. The rename itself is documented in the registry note at docs/methodology/REGISTRY.md:1486, so the canonical-name change is not a methodology defect.
  Concrete fix: none.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• No new findings. The stale Rust wording and stale global-method test docstring called out in the previous AI review are fixed at rust/src/trop.rs:1087 and tests/test_trop.py:2723.

Tech Debt

• No findings. No new TODO.md tracking appears necessary.

Security

• No findings.

Documentation/Tests

• No new blocking test gaps stood out in the diff. The deprecation/canonicalization paths are covered in tests/test_trop.py:2861 and tests/test_trop.py:2883.
• Verification note: I could not run the test suite in this environment because pytest is unavailable, and I could not import the package because numpy is not installed.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: fa25f700c3fad52a7551451e9e9cedc1f173496b

---

Overall Assessment

✅ Looks good

Re-review outcome: I do not see an unmitigated P0 or P1 in the renamed TROP local/global paths. The prior registry mismatch noted in the earlier AI review appears resolved.

Executive Summary

• The previous methodology-doc concern is fixed: the registry’s generic optimization summary now correctly separates the standard local estimator from the control-only global adaptation in docs/methodology/REGISTRY.md:L1274-L1289 and docs/methodology/REGISTRY.md:L1377-L1488.
• Deprecated alias normalization is implemented in both construction and parameter mutation in diff_diff/trop.py:L142-L175 and diff_diff/trop.py:L2669-L2692, and covered in tests/test_trop.py:L2861-L2895.
• No new NaN/SE or inline-inference regression is introduced in the touched estimator code; both global and local paths still route through safe_inference in diff_diff/trop.py:L1275-L1288 and diff_diff/trop.py:L1858-L1880.
• Two low-severity consistency items remain: private Rust helper names still use *_joint* terminology in rust/src/trop.rs:L1103-L1452, and one renamed test still has a stale alias-comparison docstring in tests/test_trop.py:L3277-L3296.
• Verification here was static only; numpy, pandas, and pytest are not installed in this environment, so I could not execute the updated tests.

Methodology

• Severity: P3 informational. Impact: none. Concrete fix: none. The twostep→local and joint→global rename is explicitly documented in docs/methodology/REGISTRY.md:L1377-L1488 and implemented as deprecated-alias normalization in diff_diff/trop.py:L66-L84 and diff_diff/trop.py:L142-L175. That makes this a documented terminology deviation rather than a methodology defect; the paper-aligned local estimator remains the per-treated-cell Algorithm 2 style procedure. citeturn4view0
• No additional methodology findings. The earlier registry inconsistency is resolved by docs/methodology/REGISTRY.md:L1274-L1289.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• Severity: P3. Impact: no behavioral change, but the Rust backend now mixes public global terminology with private *_joint* helper names in rust/src/trop.rs:L1103-L1452 and rust/src/trop.rs:L1741-L1756, which weakens the consistency goal of the rename and makes backend maintenance harder to follow. Concrete fix: rename the remaining private Rust helpers to *_global*, or add an explicit comment that only the exported/public surface was renamed.

Tech Debt

• No findings. No new TODO.md tracking looks necessary.

Security

• No findings.

Documentation/Tests

• Severity: P3. Impact: low; tests/test_trop.py:L3277-L3296 still says it “produces same results as deprecated joint,” but the body now only checks that method="global" runs and returns a positive ATT. Concrete fix: either restore the alias comparison in the test body or rewrite the docstring to match the current assertion.
• No other doc/test findings. Deprecated alias coverage is present in tests/test_trop.py:L2861-L2895, and the renamed Rust exports are exercised in tests/test_rust_backend.py:L1154-L1270.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 528faf51ab03feb822633ca8c9daf68976acd08a

---

Overall Assessment

✅ Looks good

Re-review outcome: the prior low-severity issues are addressed, and I do not see an unmitigated P0/P1 in the twostep to local / joint to global rename. The diff is behavior-preserving in estimator logic; the only methodology deviation is terminology, and that deviation is explicitly documented in docs/methodology/REGISTRY.md:L1377-L1488. This review was static only because numpy, pandas, and pytest are not installed in this environment.

Executive Summary

• The paper-facing/API terminology change is now explicitly documented as a note/deviation in docs/methodology/REGISTRY.md:L1377-L1488.
• Alias normalization is consistent in construction and parameter mutation, and get_params() returns the canonical names in diff_diff/trop.py:L140-L175 and diff_diff/trop.py:L2655-L2692.
• The renamed global path still enforces simultaneous adoption and still routes inference through safe_inference(), so I do not see a new weighting/SE/control-group regression in diff_diff/trop.py:L1109-L1277.
• Rust/Python wiring is consistent for the renamed public exports in diff_diff/_backend.py:L18-L33, rust/src/lib.rs:L45-L52, and tests/test_rust_backend.py:L1154-L1270.
• The prior re-review items are addressed: the stale global-alias test docstring is fixed in tests/test_trop.py:L3277-L3296, and the retained private Rust helper names are now explicitly scoped in rust/src/trop.rs:L1083-L1088.
• One low-severity test gap remains: I did not find a direct regression test asserting that TROP() now defaults to canonical method="local".

Methodology

• Severity: P3 informational. Impact: none. Concrete fix: none required. Renaming the public TROP method labels to local/global is a documented terminology deviation rather than a change in the estimator’s math, weighting, variance, or identification assumptions. The registry records both deprecated aliases and the rename note in docs/methodology/REGISTRY.md:L1377-L1488, and the code canonicalizes aliases without changing the underlying implementations in diff_diff/trop.py:L140-L175 and diff_diff/trop.py:L2655-L2692.
• No additional findings. The changed estimator path still enforces the simultaneous-adoption check for global and still uses safe_inference() for all inference fields in diff_diff/trop.py:L1109-L1277.

Code Quality

• No findings.

Performance

• No findings.

Maintainability

• No findings. The previous private-helper naming concern is adequately documented in rust/src/trop.rs:L1083-L1088.

Tech Debt

• No new findings.

Security

• No findings.

Documentation/Tests

• Severity: P3. Impact: low. The main public API change in this PR, zero-argument TROP() now canonicalizing to method="local", is not asserted directly in the updated tests. The new coverage checks explicit alias normalization in tests/test_trop.py:L2854-L2895 and the renamed Rust export surface in tests/test_rust_backend.py:L1154-L1270, but not the zero-argument default. Concrete fix: add a small regression test asserting TROP().method == "local" and TROP().get_params()["method"] == "local" without warnings.
• The rest of the docs/test surface looks consistent with the rename in docs/api/trop.rst:L98-L150, docs/troubleshooting.rst:L582-L623, and CHANGELOG.md:L8-L12.