Lock SDiD tau at zero for a constant gap and simplex time weights - #1209
Conversation
A zero-gap test does not exercise the cancellation that requires the weights to sum to one. Co-authored-by: Cursor <cursoragent@cursor.com>
drbenvincent
left a comment
drbenvincent
left a comment
There was a problem hiding this comment.
Executive Summary
Recommendation: approve (posted as comment because GitHub blocks self-approval on this PR).
This PR meets issue #1202's definition of done: it adds a regression test that a constant nonzero treated-minus-synthetic gap with simplex time weights yields τ = 0, leaves _compute_tau untouched, and includes the non-simplex control probe so the case is not confusable with the existing all-zero-gap test.
Review focus
- DoD vs issue #1202: constant gap
4,T_pre=5,T_post=3, simplex λ, plus off-simplex λ summing to0.5→ expected τ2.0. Matches the probe in the issue. - Math: for constant gap
c, τ =c * (1 - sum(λ)). Simplex ⇒ 0; sum(λ)=0.5 ⇒4 * 0.5 = 2. Asserts are correct. - Scope: diff is test-only under
TestComputeTau; production_compute_tauunchanged as required. - Local evidence:
pytest causalpy/tests/test_sdid_helpers.py::TestComputeTau --no-cov→ 3 passed.
Findings
None. No correctness bugs, contract violations, or DoD gaps.
Grouped by file: no issues in causalpy/tests/test_sdid_helpers.py (only changed file).
Merge readiness
Verdict: approve (content). Note the PR is still marked draft and remote CI was still pending at review time; mark ready for review / wait on green before merge. Another maintainer will need to click Approve if a formal approval is required (self-approval is disallowed).
What worked well
- Explicit contrast with
test_tau_is_zero_when_gaps_are_zeroin the docstring (zero gaps make any weights give τ=0). - Off-simplex control assertion locks in that the cancellation depends on sum(λ)=1, not on a zero gap.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pymc6_and_pymcmarketing1_migration #1209 +/- ##
===================================================================
Coverage 97.14% 97.15%
===================================================================
Files 130 130
Lines 24033 24043 +10
Branches 1349 1349
===================================================================
+ Hits 23348 23358 +10
Misses 477 477
Partials 208 208 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
drbenvincent
added
tests
…constant-gap Bring the branch up to date after #1214 synced main into the migration branch.
c0bba46
into
pymc6_and_pymcmarketing1_migration
1 participant
Closes #1202
Summary
_compute_tauunchanged.Test plan
pytest causalpy/tests/test_sdid_helpers.py::TestComputeTau --no-covMade with Cursor