You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up to #295/#297/#298 (merged in PR #301). The parametric regression families (PH, AFT, PO, AH) now share their fit pipeline via module-level plumbing — prepare_regression_fit / assemble_regression_model / the two optimiser ladders in _fit_skeleton.py — plus HazardIdentitiesMixin and LogLinearPhi. But there is still no RegressionFitter base class analogous to ParametricFitter on the distribution side: each fitter is an independent class composing those pieces.
The decision: whether to formalise a ParametricRegressionFitter base class owning the common __init__ attribute block (dist, k_dist, bounds, support, param_names, param_map, the *_dist aliases) and a template-method fit() with hooks for the optimiser strategy and reg-model builder.
Makes the implicit fitter contract explicit — real types instead of fitter: Any in _fit_skeleton.py, isinstance checks, a docs anchor. A new family (e.g. frailty) becomes correct by construction.
AFT and PO fit() bodies are now line-for-line identical except the reg-model name; a template method removes another ~60 lines.
Risks
MRO complexity: these classes already stack HazardIdentitiesMixin + TVC mixins + DataFrameRegressionMixin; the TVC mixins call into self.fit internals and a template fit() must not change what they see. This is the most likely place for a subtle break.
The irregular cases fight the template: PH's public (name, dist, phi, ...) constructor is API and cannot change; AH's optimiser is a penalty loop with a post-fit finiteness raise; the AL parameter-substitution fitter does not fit the shape and should stay outside the hierarchy. A base class with three hooks and two exceptions may just relocate complexity rather than remove it.
Serialisation coupling: _SERIALISABLE_KINDS / from_dict rebuild fitters via the public factories; constructor-signature or class-identity changes risk breaking round-trips of previously saved models. Keeping constructors frozen avoids this but constrains the design.
Diminishing returns: the remaining duplication is declarative attribute assignment that fails loudly, not logic that fails silently — the bug-generating duplication is already gone.
If done: roughly half a day; verify with the existing bit-identity reference harness plus the full TVC test suites (the TVC paths are not covered by the reference capture). Deliberately deferred from v0.17.0 since that release already carries a large amount of refactoring.
Follow-up to #295/#297/#298 (merged in PR #301). The parametric regression families (PH, AFT, PO, AH) now share their fit pipeline via module-level plumbing —
prepare_regression_fit/assemble_regression_model/ the two optimiser ladders in_fit_skeleton.py— plusHazardIdentitiesMixinandLogLinearPhi. But there is still noRegressionFitterbase class analogous toParametricFitteron the distribution side: each fitter is an independent class composing those pieces.The decision: whether to formalise a
ParametricRegressionFitterbase class owning the common__init__attribute block (dist,k_dist,bounds,support,param_names,param_map, the*_distaliases) and a template-methodfit()with hooks for the optimiser strategy and reg-model builder.Benefits
__init__blocks are hand-copies and have already drifted in small ways (which*_distaliases exist, whether the phi signature is validated). The fit-pipeline drift that produced Parametric PH: fixed={"beta_0": ...} silently fixes alpha (param map not offset by k_dist) #251 started the same way.fitter: Anyin_fit_skeleton.py,isinstancechecks, a docs anchor. A new family (e.g. frailty) becomes correct by construction.fit()bodies are now line-for-line identical except the reg-model name; a template method removes another ~60 lines.Risks
HazardIdentitiesMixin+ TVC mixins +DataFrameRegressionMixin; the TVC mixins call intoself.fitinternals and a templatefit()must not change what they see. This is the most likely place for a subtle break.(name, dist, phi, ...)constructor is API and cannot change; AH's optimiser is a penalty loop with a post-fit finiteness raise; the AL parameter-substitution fitter does not fit the shape and should stay outside the hierarchy. A base class with three hooks and two exceptions may just relocate complexity rather than remove it._SERIALISABLE_KINDS/from_dictrebuild fitters via the public factories; constructor-signature or class-identity changes risk breaking round-trips of previously saved models. Keeping constructors frozen avoids this but constrains the design.If done: roughly half a day; verify with the existing bit-identity reference harness plus the full TVC test suites (the TVC paths are not covered by the reference capture). Deliberately deferred from v0.17.0 since that release already carries a large amount of refactoring.