search

pymc-labs / pymc-marketing

Bayesian marketing toolbox in PyMC. Media Mix (MMM), customer lifetime value (CLV), buy-till-you-die (BTYD) models and more.
https://www.pymc-marketing.io/
Apache License 2.0
614 stars 148 forks source link

Discuss `fit()` Conventions #409

Open ColtAllen opened 9 months ago

ColtAllen commented 9 months ago

What should be returned when fit is called?

fitted_model = Model(data).fit() is a common convention amongst those familiar with the sklearn API, but won't work unless self is returned. Currently this method returns either None or model.fit_result for all models. The latter could confuse end users if they try to call predictive methods. Either way, all models should be consistent in this regard.

Related to https://github.com/pymc-labs/pymc-marketing/issues/318 and https://github.com/pymc-labs/pymc-marketing/issues/319, instead of calling build_model() within fit(), it'd make more sense to do it within the subclass __init__. This would allow support for external samplers and also ensure repr works properly.

ricardoV94 commented 9 months ago

I think fit should return the fit results, regardless of what sklearn does. I have seen code examples use it already, which means it would also break existing code.

Calling build_model in the CLV cases makes sense, but if the reason is only the __repr__ we can also change the __repr__ so it's not based on a built model. No strong preference here.