Fix order-dependent interaction-term lookup in DiD effect_summary - #1063
Fix order-dependent interaction-term lookup in DiD effect_summary#1063Mari1988 wants to merge 1 commit into
Conversation
_compute_statistics_did_ols matched the interaction coefficient via a
single concatenated substring, f"{group}:{post_treatment}" in label.
patsy names the interaction column with whichever variable is written
first in the formula, so a formula written as "post_treatment*group"
produces a label like "post_treatment[T.True]:group" instead of
"group:post_treatment[T.True]", which the concatenated-string check
silently failed to match.
DifferenceInDifferences.algorithm() already fixed this exact bug for
the causal_impact (point estimate) lookup in pymc-labs#994, by checking both
variable names as independent substrings instead. This separate
lookup in reporting.py, used only for the SE/CI/p-value calculation,
was never updated, so a valid reversed-order formula produced a
correct causal_impact but effect_summary() raised
"ValueError: Could not find interaction term group:post_treatment in
model".
Fixes it the same way pymc-labs#994 did: match both group_variable_name and
post_treatment_variable_name as independent substrings. Adds a
regression test asserting effect_summary() output is identical for
both formula orderings.
|
👋 Welcome to CausalPy, @Mari1988! Thank you for opening your first pull request! We're excited to have you contribute to the project. 🎉 Here are a few tips to help your PR get merged smoothly:
A maintainer will review your changes soon. Thanks for helping make CausalPy better! 🚀 💼 LinkedIn Shoutout: Once your PR is merged, we'd love to give you a shoutout on LinkedIn to thank you for your contribution! If you're interested, just drop your LinkedIn profile URL in a comment below. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1063 +/- ##
=======================================
Coverage 95.98% 95.98%
=======================================
Files 104 104
Lines 16269 16278 +9
Branches 912 913 +1
=======================================
+ Hits 15615 15624 +9
Misses 488 488
Partials 166 166 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Automated triageRecommendation: Why:
Review focus:
Confidence: high |
daimon-pymclabs
left a comment
There was a problem hiding this comment.
Thanks for this @Mari1988 — nicely scoped, and the write-up (including reproducing the pre-fix crash and confirming byte-identical output across formula orderings) makes it easy to trust. The bug is real: _compute_statistics_did_ols was the only remaining order-dependent interaction lookup, so a post_treatment*group formula gave a correct causal_impact but crashed in effect_summary(). The fix and the added regression test both look correct, and 100% patch coverage is appreciated.
One actionable suggestion (optional, non-blocking): rather than mirror #994's older independent-substring approach, you can reuse the more rigorous helper that already lives on this same object — result._is_treatment_interaction(label). It splits on :, compares exact factor names (with C(name) handling), and closes the exact false-positive gap you flagged in the "Known limitation" section (e.g. a covariate literally named post_treatment_group_flag). Since result is the DifferenceInDifferences experiment, the loop becomes:
coeff_idx = next(
(i for i, label in enumerate(result.labels)
if result._is_treatment_interaction(label)),
None,
)That would both fix this bug and unify the two lookups in one move, rather than leaving the reporting path on the weaker matcher. If you'd rather keep this PR minimal and mirror #994 exactly, that's a reasonable call too — the current fix is correct as-is. Either way, worth a maintainer eye on the SE/CI/p-value path this touches.
|
Thanks @Mari1988 — this fix is now on the v6 migration branch via #1153 (merged), which uses |
Summary
_compute_statistics_did_ols(used byDifferenceInDifferences.effect_summary()for OLS-fitted models) matched the interaction coefficient via a single concatenated substring:f"{group}:{post_treatment}" in label.patsynames the interaction column with whichever variable is written first in the formula, so a formula written aspost_treatment*groupproduces a label likepost_treatment[T.True]:groupinstead ofgroup:post_treatment[T.True], which the concatenated-string check silently fails to match.DifferenceInDifferences.algorithm()already fixed this exact bug for thecausal_impact(point estimate) lookup in Make OLS causal_impact lookup order-independent for the DiD interaction term #994, by checking both variable names as independent substrings instead. This separate lookup inreporting.py, used only for the SE/CI/p-value calculation, was never updated — so today, a valid reversed-order formula produces a correctcausal_impactbuteffect_summary()raisesValueError: Could not find interaction term group:post_treatment in model.group_variable_nameandpost_treatment_variable_nameas independent substrings.Found while reviewing #993/#994 for unrelated reasons — reading #994's fix surfaced that a sibling function had the same bug pattern it fixed, just never updated.
Known limitation (not fixed here, to keep this PR minimal and mirror #994's exact approach): this substring match could in principle produce a false positive if some other (non-interaction) coefficient's label happened to contain both variable names as substrings — e.g. a covariate literally named
post_treatment_group_flag.diff_in_diff.py's own_is_treatment_interaction()helper (used elsewhere) is more rigorous — it splits on:and requires exact factor-name equality. The two lookups have diverged in rigor; unifying them would be a reasonable, separate follow-up.Test plan
test_effect_summary_ols_did_order_independent, assertingeffect_summary()produces identical mean/ci_lower/ci_upper/p_value forgroup*post_treatmentandpost_treatment*grouppytest causalpy/tests/test_reporting.py -k did— 12 passedmake test-patch-cov— 100% patch coverage