Fix order-dependent interaction-term lookup in DiD effect_summary (port of #1063) - #1153
Merged
drbenvincent merged 1 commit intoAug 8, 2026
Conversation
_compute_statistics_did_ols located the DiD interaction coefficient by substring-matching the concatenated string "group:post_treatment" against the patsy design-matrix column names. patsy names interaction columns by formula order, so a formula written as "y ~ 1 + post_treatment*group" produces the label "post_treatment[T.True]:group" and effect_summary() raised "ValueError: Could not find interaction term ..." even though the fitted model is identical. The same lookup also failed for the categorical spelling "C(group)*post_treatment", whose label is "C(group)[T.1]:post_treatment[T.True]". Replace the substring match with the experiment's structural _is_treatment_interaction() helper (introduced in #994), which DifferenceInDifferences.algorithm() already uses to locate the causal_impact coefficient and which handles reversed factor order and the C(group) spelling. Also make the no-match error message name both variables instead of echoing the concatenated string. Regression tests assert that both formula orders and the C(group) spelling produce identical effect_summary statistics. Co-authored-by: Mari1988 <[email protected]>
Contributor
|
👋 Welcome to CausalPy, @derwells! 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 @@
## pymc6_and_pymcmarketing1_migration #1153 +/- ##
===================================================================
Coverage 97.11% 97.11%
===================================================================
Files 122 122
Lines 21634 21649 +15
Branches 1175 1175
===================================================================
+ Hits 21009 21024 +15
Misses 414 414
Partials 211 211 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Aug 8, 2026
drbenvincent
merged commit Aug 8, 2026
c5b3d4f
into
pymc6_and_pymcmarketing1_migration
16 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Port of #1063 by @Mari1988 to the migration branch.
effect_summary()on an OLSDifferenceInDifferencesresult crashed withValueError: Could not find interaction term group:post_treatment in modelwhen the formula was written asy ~ 1 + post_treatment*groupinstead ofy ~ 1 + group*post_treatment. Both formulas fit the same model. The point estimate was already order-independent (#994 fixed that inalgorithm()); only this reporting step crashed.Cause
_compute_statistics_did_olsneeds the design-matrix column index of the interaction term to compute the SE, CI, and p-value. It found that column by checking whether the concatenated string"group:post_treatment"appears in each patsy column name. patsy names interaction columns in formula order, so the reversed formula produces the labelpost_treatment[T.True]:groupand the check never matches. The categorical spellingC(group)*post_treatmentfails the same way: its label isC(group)[T.1]:post_treatment[T.True], which also does not contain the literal string.Fix
Use
_is_treatment_interaction()for the lookup instead. That helper was introduced by #994 andalgorithm()already uses it to locate thecausal_impactcoefficient. It parses each label into its factor set, strips[T.…]suffixes, accepts theC(group)spelling, and requires an exact match on{group, post_treatment}.This deviates from the diff in #1063, which matched the two variable names as independent substrings. That fixes the crash, but a label like
subgroup:post_treatmentwould also match, and since the loop takes the first hit the SE could silently come from the wrong column while the mean stays correct. The reviewer on #1063 suggested the helper for exactly this reason, and this PR adopts that suggestion. The improved error message is taken from #1063 unchanged.Part of the "stop string-matching patsy display labels" family (#993 to #1000).
Testing
test_effect_summary_ols_did_order_independent, ported verbatim from Fix order-dependent interaction-term lookup in DiD effect_summary #1063: both formula orders must produce identicaleffect_summarystatistics.test_effect_summary_ols_did_categorical_group_spelling, new: theC(group)spelling must match the plain spelling. The old lookup failed on it too.causalpy/tests/test_reporting.py: 166 passed. Full suite: 2283 passed; the only 3 failures (test_doctest_sampling.py) also fail on the untouched branch head in my local environment, so they are pre-existing and unrelated.diff-covervs this base branch, 96% threshold): 100% on all 22 changed lines.banksdataset: all three spellings return identical output (effect 20.12, 95% CI [19.12, 21.12]). Two of the three raisedValueErrorbefore the fix.Notes
PrePostNEGD._get_treatment_effect_coeffincausalpy/experiments/prepostnegd.pystill substring-matches the group name and could misfire on a column likesubgroup. Worth a follow-up issue.Checklist
prek run --all-filespassesDIFF_COVER_COMPARE_BRANCH=origin/pymc6_and_pymcmarketing1_migration)