Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom priors and likelihood in DelayedSaturated MMM #397

Merged

Conversation

cetagostini
Copy link
Contributor

@cetagostini cetagostini commented Oct 17, 2023

Hey guys, I'm applying the same configuration of the models on CLV to the MMM models, the idea is to be able to pass priors and custom distributions based on the user needs.

Everything was working properly on the mmm example notebook, except for the fit() method 😅 Here is the traceback, looks like is not directly related to the changes:

---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-26-e8614295d821> in <cell line: 1>()
----> 1 mmm.fit(X=X, y=y, target_accept=0.95, chains=4, random_seed=rng)

10 frames
/content/pymc-marketing/pymc_marketing/model_builder.py in fit(self, X, y, progressbar, predictor_names, random_seed, **kwargs)
    475             with self.model:
    476                 sampler_args = {**self.sampler_config, **kwargs}
--> 477                 self.idata = pm.sample(**sampler_args)
    478 
    479         X_df = pd.DataFrame(X, columns=X.columns)

/usr/local/lib/python3.10/dist-packages/pymc/sampling/mcmc.py in sample(draws, tune, chains, cores, random_seed, progressbar, step, nuts_sampler, initvals, init, jitter_max_retries, n_init, trace, discard_tuned_samples, compute_convergence_checks, keep_warning_stat, return_inferencedata, idata_kwargs, nuts_sampler_kwargs, callback, mp_ctx, model, **kwargs)
    649 
    650     initial_points = None
--> 651     step = assign_step_methods(model, step, methods=pm.STEP_METHODS, step_kwargs=kwargs)
    652 
    653     if nuts_sampler != "pymc":

/usr/local/lib/python3.10/dist-packages/pymc/sampling/mcmc.py in assign_step_methods(model, step, methods, step_kwargs)
    209     methods_list: List[Type[BlockedStep]] = list(methods or pm.STEP_METHODS)
    210     selected_steps: Dict[Type[BlockedStep], List] = {}
--> 211     model_logp = model.logp()
    212 
    213     for var in model.value_vars:

/usr/local/lib/python3.10/dist-packages/pymc/model/core.py in logp(self, vars, jacobian, sum)
    738             return logp_factors
    739 
--> 740         logp_scalar = pt.sum([pt.sum(factor) for factor in logp_factors])
    741         logp_scalar_name = "__logp" if jacobian else "__logp_nojac"
    742         if self.name:

/usr/local/lib/python3.10/dist-packages/pymc/model/core.py in <listcomp>(.0)
    738             return logp_factors
    739 
--> 740         logp_scalar = pt.sum([pt.sum(factor) for factor in logp_factors])
    741         logp_scalar_name = "__logp" if jacobian else "__logp_nojac"
    742         if self.name:

/usr/local/lib/python3.10/dist-packages/pytensor/tensor/math.py in sum(input, axis, dtype, keepdims, acc_dtype)
   2502     """
   2503 
-> 2504     out = Sum(axis=axis, dtype=dtype, acc_dtype=acc_dtype)(input)
   2505 
   2506     if keepdims:

/usr/local/lib/python3.10/dist-packages/pytensor/graph/op.py in __call__(self, *inputs, **kwargs)
    302         """
    303         return_list = kwargs.pop("return_list", False)
--> 304         node = self.make_node(*inputs, **kwargs)
    305 
    306         if config.compute_test_value != "off":

/usr/local/lib/python3.10/dist-packages/pytensor/tensor/elemwise.py in make_node(self, input)
   1384 
   1385     def make_node(self, input):
-> 1386         input = as_tensor_variable(input)
   1387         inp_dims = input.type.ndim
   1388         inp_dtype = input.type.dtype

/usr/local/lib/python3.10/dist-packages/pytensor/tensor/__init__.py in as_tensor_variable(x, name, ndim, **kwargs)
     47 
     48     """
---> 49     return _as_tensor_variable(x, name, ndim, **kwargs)
     50 
     51 

/usr/lib/python3.10/functools.py in wrapper(*args, **kw)
    887                             '1 positional argument')
    888 
--> 889         return dispatch(args[0].__class__)(*args, **kw)
    890 
    891     funcname = getattr(func, '__name__', 'singledispatch function')

/usr/local/lib/python3.10/dist-packages/pytensor/tensor/__init__.py in _as_tensor_variable(x, name, ndim, **kwargs)
     54     x: TensorLike, name: Optional[str], ndim: Optional[int], **kwargs
     55 ) -> "TensorVariable":
---> 56     raise NotImplementedError(f"Cannot convert {x!r} to a tensor variable.")
     57 
     58 

NotImplementedError: Cannot convert None to a tensor variable.

I'll be debugging during the week but if anyone has an idea about what could be, perhaps I can save some time 👍


📚 Documentation preview 📚: https://pymc-marketing--397.org.readthedocs.build/en/397/

@ricardoV94
Copy link
Contributor

That's a weird one. Can you do pytensor.dprint(...) of the model build by the MMM class?

@ricardoV94
Copy link
Contributor

@cetagostini Is there a test I can run to investigate?

Designed by Juan!

Co-Authored-By: Juan Orduz <[email protected]>
@cetagostini
Copy link
Contributor Author

Hey guys! @juanitorduz helped me with the test, I think everything seems to work now.

@ricardoV94 Let me know! 🚀

Copy link
Contributor

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestions, big picture looks great 👍

pymc_marketing/mmm/delayed_saturated_mmm.py Outdated Show resolved Hide resolved
pymc_marketing/mmm/delayed_saturated_mmm.py Show resolved Hide resolved
tests/mmm/test_delayed_saturated_mmm.py Outdated Show resolved Hide resolved
cetagostini and others added 2 commits December 3, 2023 03:46
@cetagostini
Copy link
Contributor Author

cetagostini commented Dec 3, 2023

@ricardoV94 Hey I tried your recommended code based on a new test for the config and it didn't work.

@juanitorduz
Copy link
Collaborator

juanitorduz commented Dec 3, 2023

We are so close @cetagostini 🔥 !

@juanitorduz
Copy link
Collaborator

It seems the regex match is still failing

AssertionError: Regex pattern "The distribution used for the likelihood is not allowed. Please, use one of the following distributions: ['Normal', 'StudentT', 'Laplace', 'Logistic', 'LogNormal', 'Wald', 'TruncatedNormal', 'Gamma', 'AsymmetricLaplace', 'VonMises']." does not match "The distribution used for the likelihood is not allowed. Please, use one of the following distributions: ['Normal', 'StudentT', 'Laplace', 'Logistic', 'LogNormal', 'Wald', 'TruncatedNormal', 'Gamma', 'AsymmetricLaplace', 'VonMises'].". Did you mean to re.escape() the regex?

I guess you need to escape the [ and ] characters 😅

+ adding Markus as co-author

Co-Authored-By: Markus Sagen <[email protected]>
@cetagostini
Copy link
Contributor Author

It seems the regex match is still failing

AssertionError: Regex pattern "The distribution used for the likelihood is not allowed. Please, use one of the following distributions: ['Normal', 'StudentT', 'Laplace', 'Logistic', 'LogNormal', 'Wald', 'TruncatedNormal', 'Gamma', 'AsymmetricLaplace', 'VonMises']." does not match "The distribution used for the likelihood is not allowed. Please, use one of the following distributions: ['Normal', 'StudentT', 'Laplace', 'Logistic', 'LogNormal', 'Wald', 'TruncatedNormal', 'Gamma', 'AsymmetricLaplace', 'VonMises'].". Did you mean to re.escape() the regex?

I guess you need to escape the [ and ] characters 😅

Yes, the string needed to be corrected. Now it's solved!

Adding everybody as co-authors

Co-Authored-By: nialloulton <[email protected]>
Co-Authored-By: Markus Sagen <[email protected]>
Co-Authored-By: Juan Orduz <[email protected]>
Co-Authored-By: Ricardo Vieira <[email protected]>
@cetagostini
Copy link
Contributor Author

cetagostini commented Dec 3, 2023

Hey there, team!

I just wanted to give a quick shoutout to everyone who contributed to this PR. It truly was a group effort and a testament to why the open-source community it's so great and moves so quickly.

It all started with an idea, which I drew inspiration from @nialloulton code. @MarkusSagen also provided some valuable insights, as he was working on something similar. @ricardoV94 supervised each step we took, ensuring that our code was not only readable but also bulletproof.

Finally after struggling with the tests, and a crazy chatgpt who was throwing out crazy nonsense (Instead of help), Mr. @juanitorduz stepped up and helped us cross the finish line with the missing unit tests.

Tremendous teamwork and support, everyone covering our backs in each area, moving the PR. I'm adding everyone as co-authors, because this PR deserves it.

Let's keep up the fantastic work!

Copy link
Contributor

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @cetagostini

@juanitorduz
Copy link
Collaborator

"This branch cannot be rebased due to conflicts" ? Which conflicts?

@ricardoV94
Copy link
Contributor

"This branch cannot be rebased due to conflicts" ? Which conflicts?

There were some merge pull commits from the main branch (instead of merge rebase)

@ricardoV94 ricardoV94 merged commit cf0a954 into pymc-labs:main Dec 4, 2023
12 checks passed
@ricardoV94
Copy link
Contributor

It's still possible to squash though (just did)

cetagostini added a commit to cetagostini/pymc-marketing that referenced this pull request Dec 4, 2023
Work based on the join contribution with @MarkusSagen on pymc-labs#397

Co-Authored-By: Markus Sagen <[email protected]>
juanitorduz pushed a commit that referenced this pull request Dec 6, 2023
…layedSaturated MMM) (#443)

* Creating Budget Allocation example

* Small change on the notebook

* Switching words

* Adding missing code

* Modifying notebook

* Changing introduction

* Adding links

* Correcting grammar v1

* changing title

* Modifying narrative v2

* Modify Narrative V3

* Changing Load Model Section

* Small grammar correction

* Updating functions and descriptions

* Adding section to handling non-fit errors

* Correcting git workflows error

* Updating notebook

* model builder changes

* Replacing dict dims

* Commenting not used params

* Adding _pre_process_prior function

* importing missing library

* Correcting error on importing

* + importing str_for_dist library

* Correcting model

* solving error on fit

* Updating code (Trying to solve dims mismatch)

* small adjustment

* Applying changes based on juanito examples

* Praying for mercy.

* debug

* modifying _create_distribution function

* Adding prior likelihood config

* Deleting hint

* Adjusting hint

* Adding docstrings

* Adding extra unit tests

* small changes

* solving error

* Adding last team feedback

* Fixing error

* New updated notebook

Work based on the join contribution with @MarkusSagen on #397

Co-Authored-By: Markus Sagen <[email protected]>

* requested changes

Co-Authored-By: Markus Sagen <[email protected]>

* model builder changes

* Replacing dict dims

* Commenting not used params

* Adding _pre_process_prior function

* importing missing library

* Correcting error on importing

* + importing str_for_dist library

* Correcting model

* solving error on fit

* Updating code (Trying to solve dims mismatch)

* small adjustment

* Applying changes based on juanito examples

* Praying for mercy.

* debug

* modifying _create_distribution function

* Adding prior likelihood config

* Deleting hint

* Adjusting hint

* Adding docstrings

* Adding extra unit tests

* small changes

* solving error

* Adding last team feedback

* Fixing error

* New updated notebook

Work based on the join contribution with @MarkusSagen on #397

Co-Authored-By: Markus Sagen <[email protected]>

* requested changes

Co-Authored-By: Markus Sagen <[email protected]>

* Adjusting

Change requested by Ricardo on 397 & Grammar on Notebook

Co-Authored-By: Ricardo Vieira <[email protected]>

* update mmm example

* small update

* Adding watermark

* Moving watermark

* small change 2

* Update mmm_example.ipynb

---------

Co-authored-by: carlosagostini <[email protected]>
Co-authored-by: Markus Sagen <[email protected]>
Co-authored-by: Ricardo Vieira <[email protected]>
@ricardoV94 ricardoV94 added enhancement New feature or request MMM labels Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request MMM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants