-
Notifications
You must be signed in to change notification settings - Fork 88
Three period ITS #575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Three period ITS #575
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
==========================================
+ Coverage 91.95% 92.60% +0.64%
==========================================
Files 33 34 +1
Lines 4776 5354 +578
==========================================
+ Hits 4392 4958 +566
- Misses 384 396 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…into three_period
…into three_period
…into three_period
|
Hi @drbenvincent , I think I've tackled all the requirements you mentionned in the issue. Please let me know your thoughts on this ! I've however did not implemented the fit_decay_model at that point. If you think it's needed, could you give me more details about it ? I didn't fully understand what were you expecting. |
| @@ -0,0 +1,442 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good. Concise. Rather than saying it extends, could you change the language so it's a bit more...
For the fixed-period intervention cases (like a marketing promotion, or public policy intervention), we can make our interrupted time series experiment aware of this by providing kwargs...
And can we change the notebook title to "Interrupted Time Series with post-intervention analysis"
Reply via ReviewNB
| @@ -0,0 +1,442 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good. Concise. Can you maybe add that persistence could in reality caused by brand awareness effects.
And can you slightly decrease the effect size. I think it is good to make it obvious that there's an effect, so it's clear in the results plots what is happening. Though the current effect size seems pretty massive.
Reply via ReviewNB
| @@ -0,0 +1,442 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get the visualisation immediately after the parameter estimation. That way the visual intuition of the results will be clear and readers can interpret the numerical results summaries with that context already loaded in their brain
Reply via ReviewNB
drbenvincent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can you update the existing demo notebook
its_lift_test. That one has a time-limited intervention period, so is ideal for showing the new functionality. So basically just add in the extra kwarg. Should get updated plot no problem. Just need to add in the calls to the additional effect summary methods. - Please add the new notebook to
/docs/source/notebooks/index.mdso that it appears in the rendered docs - Don't worry about
fit_decay_model. That's an entirely optional utility method which presumably would only be useful in specific situations, depending on what the effect decay looked like.
| "metadata": {}, | ||
| "source": [ | ||
| "Run the analysis\n", | ||
| "\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. But can you move this new note up into the intro for this notebook. Could also mention that specific examples are shown in other notebooks, but this one focuses on a point intervention - rather than a fixed-period intervention.
Reply via ReviewNB
|
Hi @drbenvincent ! I've updated everything, please let me know if you need other changes. |
drbenvincent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Looking at the API docs preview, there's a slight rendering issue of the
InterruptedTimeSeriesdoctoring. https://causalpy--575.org.readthedocs.build/en/575/api/generated/causalpy.experiments.interrupted_time_series.InterruptedTimeSeries.html#causalpy.experiments.interrupted_time_series.InterruptedTimeSeries - There's one architectural issue, but happy to set up a new issue to resolve that later.
| data : pd.DataFrame | ||
| A pandas dataframe with time series data. The index should be either | ||
| a DatetimeIndex or numeric (integer/float). | ||
| treatment_time : Union[int, float, pd.Timestamp] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be painfully explicit about treatment_time and treatment_end_time being inclusive of the dates given? That is
| self.model.calculate_cumulative_impact(self.post_intervention_impact) | ||
| ) | ||
|
|
||
| def effect_summary( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The InterruptedTimeSeries class is the only experiment class in the repository that overrides effect_summary (lines 491-664). All other experiment classes (SyntheticControl, DifferenceInDifferences, RegressionDiscontinuity, etc.) rely on the base class implementation in base.py, which auto-detects experiment type and dispatches to centralized helper functions in reporting.py. The override introduces ~170 lines of duplicated logic for computing statistics, generating tables, and generating prose that already exists in the base class and reporting module. This creates a maintenance burden where future changes to effect summary logic need to be updated in multiple places. An alternative approach would be to either: (1) add the period parameter to the base class effect_summary with ITS-specific handling, or (2) create a separate ITS-specific method like effect_summary_by_period() to make it clear this is a specialized feature. However, this concern need not be a blocker for merging—the functionality itself is solid and well-tested. We could create a follow-up issue to refactor this to align with repository patterns after the feature is merged.
Here is a first draft. I still need to add these enhancements :
5.1 Persistence Analysis Methods
analyze_persistence()5.2 Decay Model Fitting
fit_decay_model(decay_type='exponential')5.3 Comparison Period Summary
period='comparison'ineffect_summary()5.4 Enhanced Plotting
plot()methods to visually distinguish three periodstreatment_end_time📚 Documentation preview 📚: https://causalpy--575.org.readthedocs.build/en/575/