Introduce Model Adapter Layer to unify PyMC and OLS interfaces #629
Replies: 1 comment 1 reply
-
|
Hi @drbenvincent! First, thanks for this clear and neat work. Your proposed solution not only resolves the issue I mentioned, but this refactor will without a doubt bring the library to a better state, making it easier for people to contribute thanks to this layer of abstraction. To answer your questions: 1 - Public vs Internal: I don't have strong feelings about it. It might be worth keeping them internal in the documentation at first, as exposing them could clutter meaningful information for the everyday user. 2 - Lazy vs Eager: Again, no strong opinion. On one hand, I believe it would be clearer to have them instantiated in init (eager), but I can't see a reason why lazy creation would be an issue. 3 - Experiment-specific requirements: That is a complicated one to tackle. Originally, that's why I proposed an experiment-based adapter; I wasn't sure we could unify the logic across all experiments. We want to avoid moving if/else logic based on model types to if/else logic based on experiment types. In my opinion, as the library scales, the needs of experiments will become more specific. Therefore, an experiment-specific adapter could be a valuable option. I believe the best approach would be to create a default Adapter for general cases, and then for specific needs like InstrumentalVariableRegression, create child classes (e.g., SklearnAdapterIVG or PyMCAdapterIVG) that inherit from the base and only override the required methods. 4 - Plotting: From a developer's POV, I would recommend keeping them separate. It would be easier to maintain and verify, especially while "vibe coding", ensuring that modifications to one backend don't unintentionally break the other. For instance, if OLS needs an update, we should know exactly which files are affected. I'd be happy to propose a draft PR later this week to start exploring this. In the meantime, where do you think these new files should be located within the library structure? Thanks again for diving so deep into this and finding a solution! |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Based on some refactoring discussion kicked off by @JeanVanDyk, I'm adding in an idea which I generated in discussion with Claude. I think this shows promise, but more thought would need to go in to unified result types.
Tagging a few people who may have interest/thoughts: @NathanielF, @juanitorduz, @williambdean, @cetagostini.
Summary
This proposal addresses the growing complexity of model-type dispatching (
if isinstance(self.model, PyMCModel) ... else ...) throughout the experiment classes by introducing a Model Adapter Layer that provides a unified interface for both PyMC (Bayesian) and scikit-learn (OLS) models.Problem Statement
Current Architecture
CausalPy experiment classes (e.g.,
InterruptedTimeSeries,DifferenceInDifferences,SyntheticControl) accept either aPyMCModelor a scikit-learnRegressorMixinvia themodel=parameter. Internally, these classes useisinstance()checks to dispatch to appropriate code paths.The Problem
The two model types have fundamentally different interfaces:
fit()signaturefit(X, y, coords)fit(X, y)predict()return typeaz.InferenceDatanp.ndarrayxr.DataArray(2D withtreated_unitsdim)np.ndarray(typically 1D)chain,drawdims)score()returnpd.Serieswith HDIfloatThis mismatch requires extensive branching logic throughout the codebase.
Extent of the Problem
A survey of the codebase reveals model-type dispatching in every experiment class:
InterruptedTimeSeries(~10+ branch points)__init__: model fitting (lines 203-215)__init__: scoring (lines 218-223)__init__: post-period prediction (lines 230-233)__init__: impact calculation (lines 236-246)_split_post_period(): slicing predictions/impacts (lines 330-407)effect_summary(): computing statistics (lines 496-569)_comparison_period_summary(): period comparison (lines 619-744)analyze_persistence(): persistence analysis (lines 1241-1336)_bayesian_plot()/_ols_plot()methodsget_plot_data_bayesian()/get_plot_data_ols()methodsDifferenceInDifferences(~5+ branch points)__init__: model fitting (lines 137-153)__init__: causal impact extraction (lines 220-244)_bayesian_plot()/_ols_plot()methodsPanelRegression(~5+ branch points)__init__: model fitting (lines 220-231)get_plot_data_bayesian()/get_plot_data_ols()plot_unit_effects(): coefficient extractionplot_trajectories(): prediction handlingplot_residuals(): data extractionOther Experiment Classes
Similar patterns exist in
SyntheticControl,RegressionDiscontinuity,RegressionKink, andPrePostNEGD.Consequences
Proposed Solution: Model Adapter Layer
Architecture Overview
Introduce an adapter layer that wraps both model types to provide a unified interface:
Unified Result Types
Adapter Implementations
Factory Function
Simplified Experiment Code
With adapters, experiment classes become cleaner:
Advantages
1. No Breaking API Changes
Users continue to pass
model=cp.pymc_models.LinearRegression()ormodel=LinearRegression(). The adapter layer is purely internal.2. Eliminates Pervasive Branching
Instead of 10+
isinstancechecks per experiment class, there's one check increate_adapter().3. Centralized Model Logic
All PyMC-specific and sklearn-specific behavior lives in the adapter classes, not scattered across experiment classes.
4. Easier Extension
Adding a new backend (e.g., Nutpie, NumPyro, statsmodels) requires only implementing a new adapter class, not modifying every experiment.
5. Better Testability
6. Improved Documentation
Backend-specific behavior is documented in adapter classes, not buried in experiment implementation details.
7. Type Safety
The unified result types (
PredictionResult,ImpactResult) provide clear contracts that can be type-checked.Disadvantages / Considerations
1. Increased Abstraction
Adds a layer of indirection. Developers need to understand the adapter pattern to contribute.
2. Migration Effort
Significant refactor touching all experiment classes. Estimated effort: 2-3 days for core implementation, plus testing.
3. Result Type Overhead
The unified result types add some overhead compared to raw numpy arrays or InferenceData. This should be negligible in practice since MCMC sampling dominates runtime.
4. Potential Edge Cases
Some experiment classes may have unique requirements that don't fit the unified interface cleanly. May need adapter extensions or experiment-specific handling.
5. Learning Curve
New contributors need to understand the adapter pattern, though this is offset by cleaner experiment code.
Implementation Plan
Phase 1: Core Infrastructure
ModelAdapterprotocol and result dataclassesPyMCAdapterandSklearnAdapterPhase 2: Migrate Experiment Classes
InterruptedTimeSeriesas proof of conceptPhase 3: Cleanup
isinstancechecks_bayesian_plot/_ols_plotwhere possible (usingis_bayesianflag)Relationship to Issue #624
This proposal supersedes the approach suggested in #624, which proposed:
interrupted_time_series_pymc.py,interrupted_time_series_ols.py)backend="pymc"parameter instead of passing model objectsThe adapter approach is preferred because:
Questions for Discussion
__init__)?InstrumentalVariableRegressionhas a uniquefit()signature)?_bayesian_plot/_ols_plot)?Beta Was this translation helpful? Give feedback.
All reactions