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
718 stars 203 forks source link

Consolidate `ModelBuilder` InferenceData getters (and setters) #742

Open wd60622 opened 5 months ago

wd60622 commented 5 months ago

Think the ModelBuilder class itself could just get the getters (and setters) for:

They are defined here for mmm: https://github.com/pymc-labs/pymc-marketing/blob/664b5caef1f20785a8f73cac3c4f83a13ceeeee7/pymc_marketing/mmm/base.py#L266-L290) and here for clv: https://github.com/pymc-labs/pymc-marketing/blob/664b5caef1f20785a8f73cac3c4f83a13ceeeee7/pymc_marketing/clv/models/basic.py#L242-L256

Would the clv modules benefit from this? @ColtAllen I'm leaning more toward standardization of code and having informative error messages

ColtAllen commented 5 months ago

Would the clv modules benefit from this? @ColtAllen I'm leaning more toward standardization of code and having informative error messages

CLVModel overwrites quite a few of the inherited ModelBuilder methods because the latter is built around an "X,Y" variable convention that doesn't apply to the CLV models. That said, the idata methods could benefit from some cleanup, particularly if it gets rid of the following warning that pops up during testing:

~/site-packages/arviz/data/inference_data.py:1538: UserWarning: The group fit_data is not defined in the InferenceData scheme

This warning is probably extraneous from the ModelBuilder class, but also worth mentioning:

~/site-packages/pymc/model/core.py:518: FutureWarning: All coords are now mutable by default. coords_mutable will be removed in a future release.

wd60622 commented 5 months ago

This warning is probably extraneous from the ModelBuilder class, but also worth mentioning:

~/site-packages/pymc/model/core.py:518: FutureWarning: All coords are now mutable by default. coords_mutable will be removed in a future release.

Yeah, I think we should get rid of these. Have this in mind #669

wd60622 commented 5 months ago

CLVModel overwrites quite a few of the inherited ModelBuilder methods because the latter is built around an "X,Y" variable convention that doesn't apply to the CLV models. That said, the idata methods could benefit from some cleanup, particularly if it gets rid of the following warning that pops up during testing:

Maybe there can be two versions of the ModelBuilder? There can be some base class that will handle this common logic of getters and setters. Also anything else that is common between the two use-cases. Then two child classes with the X at initialize and X,y in the methods. Just an initial thought...