From 66ee1444d78f706f88db5ec1a92d0a70cecc651b Mon Sep 17 00:00:00 2001 From: anevolbap Date: Sun, 17 May 2026 19:32:56 -0400 Subject: [PATCH] refactor(clv): drop _prior suffix backwards-compat shim Remove the model_config rename loop in CLVModel.__init__ and the _rename_posterior_variables hook in build_from_idata, plus the matching tests. This was added in #1498 as a v1.0 deprecation and is one of the items flagged for removal in #1516. Also rename the _prior-suffixed keys in BetaGeoModel and ModifiedBetaGeoModel covariate convergence tests, and in the bg_nbg_covariates_test_issues dev notebook, which previously relied on the shim. Refs #1516 --- .../dev/bg_nbg_covariates_test_issues.ipynb | 4 +-- pymc_marketing/clv/models/basic.py | 27 ----------------- tests/clv/models/test_basic.py | 29 ------------------- tests/clv/models/test_beta_geo.py | 28 +++++++++--------- tests/clv/models/test_modified_beta_geo.py | 28 +++++++++--------- 5 files changed, 30 insertions(+), 86 deletions(-) diff --git a/docs/source/notebooks/clv/dev/bg_nbg_covariates_test_issues.ipynb b/docs/source/notebooks/clv/dev/bg_nbg_covariates_test_issues.ipynb index 1d4dfabe6..854f8e53c 100644 --- a/docs/source/notebooks/clv/dev/bg_nbg_covariates_test_issues.ipynb +++ b/docs/source/notebooks/clv/dev/bg_nbg_covariates_test_issues.ipynb @@ -318,8 +318,8 @@ "dropout_covariate_cols = [\"dropout_cov\"]\n", "\n", "non_nested_priors = dict(\n", - " a_prior=Prior(\"Uniform\", lower=0, upper=1),\n", - " b_prior=Prior(\"Uniform\", lower=0, upper=1),\n", + " a=Prior(\"Uniform\", lower=0, upper=1),\n", + " b=Prior(\"Uniform\", lower=0, upper=1),\n", ")\n", "covariate_config = dict(\n", " purchase_covariate_cols=purchase_covariate_cols,\n", diff --git a/pymc_marketing/clv/models/basic.py b/pymc_marketing/clv/models/basic.py index 12943e31a..2c6ae47c3 100644 --- a/pymc_marketing/clv/models/basic.py +++ b/pymc_marketing/clv/models/basic.py @@ -54,18 +54,6 @@ def __init__( model_config = model_config or {} - deprecated_keys = [key for key in model_config if key.endswith("_prior")] - for key in deprecated_keys: - new_key = key.replace("_prior", "") - warnings.warn( - f"The key '{key}' in model_config is deprecated and will be removed in future versions." - f"Use '{new_key}' instead.", - DeprecationWarning, - stacklevel=2, - ) - - model_config[new_key] = model_config.pop(key) - super().__init__(model_config, sampler_config) # Parse model config after merging with defaults @@ -310,7 +298,6 @@ def build_from_idata(cls, idata: az.InferenceData) -> None: model = cls(**kwargs) model.idata = idata - model._rename_posterior_variables() model.data = idata.fit_data.to_dataframe() model.build_model(model.data) # type: ignore @@ -324,20 +311,6 @@ def build_from_idata(cls, idata: az.InferenceData) -> None: raise DifferentModelError(msg) return model - # TODO: Remove for v1.0 - def _rename_posterior_variables(self): - """Rename variables in the posterior group to remove the _prior suffix. - - This is used to support the old model configuration format, which used - to include a _prior suffix for each parameter. - """ - prior_vars = [ - var for var in self.idata.posterior.data_vars if var.endswith("_prior") - ] - rename_dict = {var: var.replace("_prior", "") for var in prior_vars} - self.idata.posterior = self.idata.posterior.rename(rename_dict) - return self.idata.posterior - def thin_fit_result(self, keep_every: int): """Return a copy of the model with a thinned fit result. diff --git a/tests/clv/models/test_basic.py b/tests/clv/models/test_basic.py index 8573cf035..f94ac9faa 100644 --- a/tests/clv/models/test_basic.py +++ b/tests/clv/models/test_basic.py @@ -309,35 +309,6 @@ def test_model_config_warns(self) -> None: "x": Prior("StudentT", mu=0, sigma=5, nu=15), } - def test_backwards_compatibility_with_old_config(self): - model = CLVModelTest() - model.build_model() - - old_posterior = from_dict(posterior={"alpha_prior": np.random.randn(2, 100)}) - set_model_fit(model, old_posterior) - assert "alpha_prior" in model.idata.posterior - - save_path = "test_model" - model.save(save_path) - - loaded_model = CLVModelTest.load(save_path) - - assert "alpha" in loaded_model.idata.posterior - assert "alpha_prior" not in loaded_model.idata.posterior - - os.remove("test_model") - - def test_deprecation_warning_on_old_config(self): - old_model_config = { - "x_prior": {"dist": "Normal", "kwargs": {"mu": 0, "sigma": 1}} - } - with pytest.warns( - DeprecationWarning, match=r"The key 'x_prior' in model_config is deprecated" - ): - model = CLVModelTest(model_config=old_model_config) - - assert model.model_config == {"x": Prior("Normal", mu=0, sigma=1)} - def test_validate_cols_reports_all_missing_columns(self): """Test _validate_cols raises a single ValueError listing all missing columns.""" required = ("customer_id", "frequency", "recency", "T") diff --git a/tests/clv/models/test_beta_geo.py b/tests/clv/models/test_beta_geo.py index 0ea6cfece..6b7838072 100644 --- a/tests/clv/models/test_beta_geo.py +++ b/tests/clv/models/test_beta_geo.py @@ -712,8 +712,8 @@ def setup_class(cls): purchase_covariate_cols = ["purchase_cov1", "purchase_cov2"] dropout_covariate_cols = ["dropout_cov"] non_nested_priors = dict( - a_prior=Prior("Beta", alpha=20, beta=20), - b_prior=Prior("Beta", alpha=20, beta=20), + a=Prior("Beta", alpha=20, beta=20), + b=Prior("Beta", alpha=20, beta=20), ) covariate_config = dict( purchase_covariate_cols=purchase_covariate_cols, @@ -1004,12 +1004,12 @@ def test_covariate_model_convergence_a_b(self): ) # The default parameter priors are very informative. We use something broader here custom_priors = { - "r_prior": Prior("Exponential", scale=10), - "alpha_prior": Prior("Exponential", scale=10), - "a_prior": Prior("Exponential", scale=10), - "b_prior": Prior("Exponential", scale=10), - "purchase_coefficient_prior": Prior("Normal", mu=0, sigma=4), - "dropout_coefficient_prior": Prior("Normal", mu=0, sigma=4), + "r": Prior("Exponential", scale=10), + "alpha": Prior("Exponential", scale=10), + "a": Prior("Exponential", scale=10), + "b": Prior("Exponential", scale=10), + "purchase_coefficient": Prior("Normal", mu=0, sigma=4), + "dropout_coefficient": Prior("Normal", mu=0, sigma=4), } new_model = BetaGeoModel( synthetic_data, @@ -1046,12 +1046,12 @@ def test_covariate_model_convergence_phi_kappa(self): ) # The default parameter priors are very informative. We use something broader here custom_priors = { - "r_prior": Prior("Exponential", scale=10), - "alpha_prior": Prior("Exponential", scale=10), - "phi_dropout_prior": Prior("Uniform", lower=0, upper=1), - "kappa_dropout_prior": Prior("Pareto", alpha=1, m=1), - "purchase_coefficient_prior": Prior("Normal", mu=0, sigma=5), - "dropout_coefficient_prior": Prior("Normal", mu=0, sigma=5), + "r": Prior("Exponential", scale=10), + "alpha": Prior("Exponential", scale=10), + "phi_dropout": Prior("Uniform", lower=0, upper=1), + "kappa_dropout": Prior("Pareto", alpha=1, m=1), + "purchase_coefficient": Prior("Normal", mu=0, sigma=5), + "dropout_coefficient": Prior("Normal", mu=0, sigma=5), } new_model = BetaGeoModel( synthetic_data, diff --git a/tests/clv/models/test_modified_beta_geo.py b/tests/clv/models/test_modified_beta_geo.py index 39346de56..71ca4b4c5 100644 --- a/tests/clv/models/test_modified_beta_geo.py +++ b/tests/clv/models/test_modified_beta_geo.py @@ -516,8 +516,8 @@ def setup_class(cls): purchase_covariate_cols = ["purchase_cov1", "purchase_cov2"] dropout_covariate_cols = ["dropout_cov"] non_nested_priors = dict( - a_prior=Prior("Beta", alpha=20, beta=20), - b_prior=Prior("Beta", alpha=20, beta=20), + a=Prior("Beta", alpha=20, beta=20), + b=Prior("Beta", alpha=20, beta=20), ) covariate_config = dict( purchase_covariate_cols=purchase_covariate_cols, @@ -808,12 +808,12 @@ def test_covariate_model_convergence_a_b(self): ) # The default parameter priors are very informative. We use something broader here custom_priors = { - "r_prior": Prior("HalfFlat"), - "alpha_prior": Prior("HalfFlat"), - "a_prior": Prior("HalfFlat"), - "b_prior": Prior("HalfFlat"), - "purchase_coefficient_prior": Prior("Normal", mu=0, sigma=4), - "dropout_coefficient_prior": Prior("Normal", mu=0, sigma=4), + "r": Prior("HalfFlat"), + "alpha": Prior("HalfFlat"), + "a": Prior("HalfFlat"), + "b": Prior("HalfFlat"), + "purchase_coefficient": Prior("Normal", mu=0, sigma=4), + "dropout_coefficient": Prior("Normal", mu=0, sigma=4), } new_model = ModifiedBetaGeoModel( model_config=self.model_with_covariates.model_config | custom_priors, @@ -849,12 +849,12 @@ def test_covariate_model_convergence_phi_kappa(self): ) # The default parameter priors are very informative. We use something broader here custom_priors = { - "r_prior": Prior("HalfFlat"), - "alpha_prior": Prior("HalfFlat"), - "phi_dropout_prior": Prior("Uniform", lower=0, upper=1), - "kappa_dropout_prior": Prior("Pareto", alpha=1, m=1), - "purchase_coefficient_prior": Prior("Flat"), - "dropout_coefficient_prior": Prior("Flat"), + "r": Prior("HalfFlat"), + "alpha": Prior("HalfFlat"), + "phi_dropout": Prior("Uniform", lower=0, upper=1), + "kappa_dropout": Prior("Pareto", alpha=1, m=1), + "purchase_coefficient": Prior("Flat"), + "dropout_coefficient": Prior("Flat"), } new_model = ModifiedBetaGeoModel( synthetic_data,