Refactoring ITS summary method

[JeanVanDyk]

Hi everyone,

I removed the override of effect_summary in interrupted_time_series.py, which removed in total ~100 lines of duplicated logic.

While reviewing the current implementation, I have three architectural concerns that I’d like to discuss before we merge:

1. Relocation of _comparison_period_summary : The _comparison_period_summary method is currently located in iterrupted_time_series.py. Given that its primary responsibility is formatting data for output, it feels like it might be a better fit for reporting.py. Moving it there would help centralize our reporting logic and make it easier to find in the future, such as the _effect_summary_{experiment type} we already find there.

2. Refactoring base.py (Lines 200–375): The logic currently sitting between lines 200 and 375appears to be exclusively specific to experiments :

if experiment_type == "`XXX":
            return _effect_summary_XXX(
                self,
                direction=direction,
                alpha=alpha,
                min_effect=min_effect,
            )

To keep base.py clean and maintainable, I believe it would be better to move this code into its corresponding experiment-specific class. This ensures our base class remains generic and doesn't "leak" logic from individual experiments.

We could however keep lines 314-375 into another method which would be called both by SyntheticControl and InterruptedTimeSeries to prevent duplicate logic.

4. Test Organization: I am adding extensive tests for these changes. Following the patterns I've seen in the repo, I’ve placed these in the tests/ folder. However, to prevent this folder from growing out of control as we scale, I’d like to propose we organize the tests/ directory to mirror our source directory structure (e.g., tests/models/ and tests/utils/).

Let me know what you think! I’m happy to handle the refactoring if we’re in agreement on the direction.

---

📚 Documentation preview 📚: https://causalpy--606.org.readthedocs.build/en/606/

[cursor]

PR Summary

Major refactor to localize effect summarization and enhance ITS functionality.

• Base: Make effect_summary keyword-only and abstract; remove experiment/model-type dispatch logic.
• ITS: Implement effect_summary with period ("intervention"/"post"/"comparison") and prefix, including _comparison_period_summary; supports PyMC/OLS via reporting helpers.
• DiD/PrePostNEGD/RD/RKink/SyntheticControl: Add experiment-specific effect_summary methods (delegating to reporting helpers where applicable); SyntheticControl supports windows, cumulative/relative options, and prose/table generation.
• IV/IPW: Define effect_summary stubs that raise NotImplementedError.
• Tests: Add tests/test_its_effect_summary.py covering PyMC/OLS, datetime/integer indices, windows, parameters, error cases, and consistency.
• Misc: Update interrogate badge SVG (96.3% → 96.6%).

^{Written by Cursor Bugbot for commit fba5a61. This will update automatically on new commits. Configure here.}

[codecov]

Codecov Report

❌ Patch coverage is 97.92627% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.77%. Comparing base (fbf8a61) to head (fba5a61).

Files with missing lines	Patch %	Lines
causalpy/experiments/interrupted_time_series.py	87.17%	2 Missing and 3 partials ⚠️
causalpy/experiments/synthetic_control.py	81.81%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #606      +/-   ##
==========================================
+ Coverage   93.27%   93.77%   +0.50%     
==========================================
  Files          37       38       +1     
  Lines        5632     5991     +359     
  Branches      367      367              
==========================================
+ Hits         5253     5618     +365     
+ Misses        248      241       -7     
- Partials      131      132       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[drbenvincent]

Thanks for this contribution! The removal of ~175 lines of duplicated code is a win, and the test coverage you've added is excellent.

I also appreciate you raising the architectural concerns proactively. You've identified the same issues I've been thinking about: the base class is starting to accumulate experiment-specific logic, and _comparison_period_summary feels misplaced.

My main concern with the current approach: The base class now accesses ITS-specific attributes like datapost, treatment_time, and treatment_end_time - note the # type: ignore[attr-defined] comments, which are a signal that we're reaching into subclass territory. This creates an awkward coupling where BaseExperiment knows about concepts that only exist in ITS.

This feels like a good opportunity to rethink the reporting architecture more holistically rather than patching incrementally.
I see two promising directions:

Option 1: Experiment-owned effect_summary() with shared utilities
Each experiment class owns its effect_summary() implementation entirely. The base class either has no implementation or declares it abstract. reporting.py becomes a pure utility module—stateless functions that take data and return results, with no knowledge of experiment objects. ITS handles its three-period logic; SC handles multi-unit logic; scalar experiments (DiD, RD, RKink) handle theirs. Code reuse happens through utility function calls, not inheritance.

Option 2: Strategy/Generator pattern
Create an EffectSummaryGenerator hierarchy in reporting.py. Each experiment instantiates the appropriate generator (e.g., TimeSeriesEffectGenerator, ThreePeriodEffectGenerator). This keeps reporting logic centralized while avoiding base class bloat.

Both approaches keep the base class clean and make experiment-specific logic explicit. I'd lean toward Option 1 for its simplicity—it's easier to understand where logic lives, and pure utility functions are straightforward to test.

What do you think? Happy to discuss further or pair on a refactor if you're interested in tackling this. Your three-period work and tests would slot in nicely once the architecture is settled.

[JeanVanDyk]

Thanks for the detailed feedback! I’m glad to see the reduction in code duplication.

I completely agree with your point regarding the # type: ignore[attr-defined] signals. It’s clear that BaseExperiment shouldn't be burdened with knowing the internals of treatment_time or the specific three-period logic of an ITS.

Decision: Moving forward with Option 1
I’ve decided to go with Option 1. It feels like the most "Pythonic" and straightforward path forward. By making effect_summary() experiment-owned and treating reporting.py as a stateless utility module, we gain a much clearer mental model of where logic lives. It effectively removes the "reach-back" coupling where the base class has to guess what the subclasses are doing.

Future Refinements:

• Logic Deduplication: If we identify shared logic across different experiment types (like standard formatting for confidence intervals), we'll have the option to move those into the reporting.py utility file to keep the experiment classes focused on their specific data structures.
• Summary Generator: While I'm starting with Option 1 for its simplicity, having this cleaner separation now leaves the door open for a more formal Generator pattern later if the reporting logic grows significantly more complex.