Allowing Custom Ordering Adstock & Saturation

[cetagostini-wise]

Hello team, as we all know we now have a large number of saturation and lagging functions, however our code was only forcing a specific type.

Create a folder with the lagging and saturation components, this will allow users to define the function they want and have it mapped.

Currently we could choose the function in the following way:

mmm = DelayedSaturatedMMM(
    model_config = my_model_config,
    sampler_config = my_sampler_config,
    lagging_function = "geometric",
    saturation_function = "logistic",
    date_column="date_week",
    channel_columns=["x1", "x2"],
    control_columns=[
        "event_1",
        "event_2",
        "t",
    ],
    adstock_max_lag=8,
    yearly_seasonality=2,
)

Important: For now the model continues applying lagging and then saturation, this cannot be modified yet but after doing this merge, I will start working on it in another PR.

Right now the model works perfectly with the following functions: a) Saturation:

• Hill
• Michaelis Menten
• Logistic

b) Lagging:

• Weibull (PDF & CDF)
• Geometric

Any of these combinations train properly. Important, the function plot_channel_contributions_grid is not available right now because was hard coded to work only with the main principal lagging and saturation functions. I need to think how to make it general, that being said, I do not consider that this should be a blocker to do merge, because it is only to display the saturation functions only. We can work on an improvement in another PR.

Description

Related Issue

• [x] Closes #602
• [x] Related to #551

Checklist

• [x] Checked that the pre-commit linting/style checks pass
• [ ] Included tests that prove the fix is effective or that the new feature works
• [x] Added necessary documentation (docstrings and/or example notebooks)
• [ ] If you are a pro: each commit corresponds to a relevant logical change

Modules affected

• [x] MMM
• [ ] CLV

Type of change

• [x] New feature / enhancement
• [ ] Bug fix
• [ ] Documentation
• [ ] Maintenance
• [ ] Other (please specify):

---

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

[cetagostini]

Now is possible to pass an string to the class or a custom component class, e.g:

mmm = DelayedSaturatedMMM(
    model_config = my_model_config,
    sampler_config = my_sampler_config,
    adstock = "geometric",
    saturation = LogisticSaturationComponent,
    date_column="date_week",
    channel_columns=["x1", "x2"],
    control_columns=[
        "event_1",
        "event_2",
        "t",
    ],
    adstock_max_lag=8,
    yearly_seasonality=2,
)

If the user wants to allow for a custom function (Component) then he must create a class which contains a variable mapping property and a default config property. In order to allow other functions as lift test integration work.

[cetagostini]

I have still the issue of:

Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/envs/632/lib/python3.10/site-packages/sphinx/config.py", line 358, in eval_config_file
    exec(code, namespace)  # NoQA: S102
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/checkouts/632/docs/source/conf.py", line 5, in <module>
    import pymc_marketing  # isort:skip
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/envs/632/lib/python3.10/site-packages/pymc_marketing/__init__.py", line 1, in <module>
    from pymc_marketing import clv, mmm
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/envs/632/lib/python3.10/site-packages/pymc_marketing/mmm/__init__.py", line 1, in <module>
    from pymc_marketing.mmm import base, delayed_saturated_mmm, preprocessing, validating
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/envs/632/lib/python3.10/site-packages/pymc_marketing/mmm/delayed_saturated_mmm.py", line 22, in <module>
    from pymc_marketing.mmm.models.components.lagging import _get_lagging_function
ModuleNotFoundError: No module named 'pymc_marketing.mmm.models.components'

@juanitorduz @wd60622

If you don't find the separation into folders something practical then I'm going to leave everything in the main to avoid it. If you want to modify, feel free. Anything to move quickly.

I only do it because I would like to maintain some order and possibly have a folder with different models like in the CLV model.

[williambdean]

The lift tests will need something like this:

variable_mapping = {
    "lam": "saturation_lambda",
    "beta": "saturation_beta",
}

def saturation_function(x, beta, lam):
    return beta * logistic_saturation(x, lam)

add_lift_measurements_to_likelihood(
    df_lift_test,
    variable_mapping,
    saturation_function=saturation_function,
    model=model,
    dist=dist,
    name=name,
)

The variable_mapping is already used, but the saturation_function would have to be exposed in each class as well

EDIT: Opening up an PR into your branch carlos: https://github.com/cetagostini-wise/pymc-marketing/pull/1

[williambdean]

I have still the issue of:

Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/envs/632/lib/python3.10/site-packages/sphinx/config.py", line 358, in eval_config_file
    exec(code, namespace)  # NoQA: S102
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/checkouts/632/docs/source/conf.py", line 5, in <module>
    import pymc_marketing  # isort:skip
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/envs/632/lib/python3.10/site-packages/pymc_marketing/__init__.py", line 1, in <module>
    from pymc_marketing import clv, mmm
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/envs/632/lib/python3.10/site-packages/pymc_marketing/mmm/__init__.py", line 1, in <module>
    from pymc_marketing.mmm import base, delayed_saturated_mmm, preprocessing, validating
  File "/home/docs/checkouts/readthedocs.org/user_builds/pymc-marketing/envs/632/lib/python3.10/site-packages/pymc_marketing/mmm/delayed_saturated_mmm.py", line 22, in <module>
    from pymc_marketing.mmm.models.components.lagging import _get_lagging_function
ModuleNotFoundError: No module named 'pymc_marketing.mmm.models.components'

@juanitorduz @wd60622

If you don't find the separation into folders something practical then I'm going to leave everything in the main to avoid it. If you want to modify, feel free. Anything to move quickly.

I only do it because I would like to maintain some order and possibly have a folder with different models like in the CLV model.

I have not experienced this while working in my branch / locally. This is in the CI/CD right?

[williambdean]

Action items:

• [x] Run the notebooks @cetagostini
• [ ] Point out new functionality in notebook @cetagostini
• [ ] Add docstring example for defining a custom saturation function @wd60622
• [x] Consider renaming DelayedSaturatedMMM -> MMM
• [x] Check the default priors for all of the adstock and saturation functions
• [x] Fix the commented out code in Test @cetagostini
• [x] Fix the forward pass used in various methods
• [ ] Create various tests for new saturation function behavior @wd60622
• [x] Model config backward compat

*Working list

[review-notebook-app[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[cetagostini]

pre-commit.ci autofix

[cetagostini]

pre-commit.ci autofix

[williambdean]

Lots of good stuff here. Think we got some more items to tackle Sadly, I think the name change should be separate from this PR. Though having flexible saturation and adstock which makes the "Delayed" "Saturated" hints a name change.

Would love to hear the overall thoughts from the Bayesian wizard @juanitorduz

[juanitorduz]

@cetagostini-wise @wd60622 I will review this PR tomorrow (it is my top priority). Today, I looked into the older ones that are ready to be merged soon 🙇 .

[juanitorduz]

Hey @wd60622 @cetagostini, I had a quick look! It looks super nice, and the code is much cleaner! Thank you 🙏 I left some initial questions and comments later.

One thing I do not understand is the name Philly? IMO can be very confusing and does not seem to relate to MMM.... why is the name compared to a simple MMM?

[cetagostini]

Hey team,

I address a few of the recommendations. I ran the notebook and it works properly.

I suggest we keep the class DelayedSaturatedMMM and add a deprecation warning. We can say we will remove it in the next release (e.g. 0.6)

Agree with this! @juanitorduz

Sadly, I think the name change should be separate from this PR. Though having flexible saturation and adstock which makes the "Delayed" "Saturated" hints a name change.

Sorry, my English is a bit bad lately @wd60622. But I understood that the name fascinates you and you are very excited about it but you don't like to express it in public! 🙂

One thing I do not understand is the name Philly? IMO can be very confusing and does not seem to relate to MMM.... why is the name compared to a simple MMM?

Regarding the background of the name, details from the internal (Top secret for now) 😅 But my opinion is that the big and great names don't have to be related to the product. What do apples have to do with technology? or What did "Google" mean even before it was a company? I'm sure a name like TheWorldsBestSearcher would never have suited them 💭

Current missing items:

• [ ] Add docstring example for defining a custom saturation function @wd60622
• [ ] Create various tests for new saturation function behavior @wd60622
• [ ] Changing folder structures @cetagostini
• [ ] Add deprecation warning @cetagostini
• [ ] Rename back the package @cetagostini

Feel free to modify or recommend suggestion if needed 💪🏻

[juanitorduz]

@cetagostini @wd60622 This is looking great 🎸 !

As this is such a big refactor, shall we also re-un all the MMM notebooks (example, optimization and lift tests) to update the api if needed and to see if everything works as expected from the documentation perspective?

[cetagostini-wise]

Tomorrow I'll be going over all comments!

[williambdean]

I will make some tests and add to docs after the weekend!

[williambdean]

FYI: https://github.com/cetagostini-wise/pymc-marketing/pull/3

[williambdean]

I've added some docstrings https://github.com/cetagostini-wise/pymc-marketing/pull/4

[williambdean]

I think that this will close this issue: #383 Someone else might know a bit more context about that

[williambdean]

I've added some docstrings

https://github.com/cetagostini-wise/pymc-marketing/pull/4

Able to merge these in @cetagostini ?

Think we are gettig closer. Any items you need help with?

[twiecki]

[docs/source/notebooks/mmm/mmm_example copy.ipynb](https://github.com/pymc-labs/pymc-marketing/pull/632/files#diff-185a5ca13ee863d30d3ef55846e1efe76b397b8fa5ee51a0cd02baf40e09c398) this file should probably not show up here.

[cetagostini]

[docs/source/notebooks/mmm/mmm_example copy.ipynb](https://github.com/pymc-labs/pymc-marketing/pull/632/files#diff-185a5ca13ee863d30d3ef55846e1efe76b397b8fa5ee51a0cd02baf40e09c398) this file should probably not show up here.

Indeed, I have this file to test my code, will be deleted before merge @twiecki

[cetagostini]

I've added some docstrings cetagostini-wise#4

Able to merge these in @cetagostini ? Think we are gettig closer. Any items you need help with?

Not now, I'm finishing the plots this week, you can start to review if you want 🙌🏻 I should make a few plots, and will start to work on the new test, and pre-commits.

[juanitorduz]

@cetagostini @wd60622 this is looking great! The code is so nice to read! For the next steps, I suggest:

• Update branch (click the button XD)
• Address comments and questions
• Add missing type hints (and make MyPy happy)
• Add missing unittests.
• Make pre-commit-happy.
• Rerun the MMM notebooks (all, to make sure we are not breaking something and also to see how this will look from the user perspective). Also, to remove the mmm_exaple_copy notebook ;)

After these steps are done, I can give another review round 🙌 . WDYT?

[cetagostini]

Hey team, majority of comments address.

All test are good except some plot test for 3.10. Maybe one of you can help with it?

Otherwise, both notebooks ran perfectly and the new adjustments feels pretty cool, would love if you have time to read the adjusted notebook for budget allocation.

Let me know! cc: @juanitorduz @wd60622

[williambdean]

Many of the failures are from the mmm test_plotting due to plot_direct_contribution_curves and plot_channel_parameter which at first glance is confusing unless those are still commented out. Are you able to run that test file locally? i.e.

pytest tests/mmm/test_plotting.py

This could have something to do with the mixin order used in the test or if the methods have moved to a different class that is from the mixin used in the test.

test_get_valid_distribution and test_get_invalid_distribution use the old _get_distribution method directly. Can you pull out that test logic and use the new get_distribution_from_dict

I'd have to look into the test_allocate_budget_zero_total a bit more.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-27T09:55:11Z ----------------------------------------------------------------

Why the the HDI larger in this updated version?

wd60622 commented on 2024-05-29T12:31:31Z ----------------------------------------------------------------

Is there a script to make the model.nc that you are using?

wd60622 commented on 2024-05-29T12:36:16Z ----------------------------------------------------------------

Or is it just the original one being renamed? The loaded model has the posterior / Dataset, right? Or do these plots require the model graph?

cetagostini commented on 2024-06-03T20:33:08Z ----------------------------------------------------------------

My guess here is, the following intervals are coming from the real model, and not being estimated by scipy based on the posterior. Meaning, probably even when we used the posterior intervals to estimate the parameters, the results were not necessarily representing the whole uncertainty.

cetagostini commented on 2024-06-03T20:55:17Z ----------------------------------------------------------------

Regarding the model, run the example notebook. I added a "save" example which generates this model.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-27T09:55:12Z ----------------------------------------------------------------

The formula is not rendering well, see https://pymc-marketing--632.org.readthedocs.build/en/632/notebooks/mmm/mmm_budget_allocation_example.html

wd60622 commented on 2024-06-08T06:45:00Z ----------------------------------------------------------------

+1

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-27T09:55:13Z ----------------------------------------------------------------

Add title and ; and the end of the plot

cetagostini commented on 2024-06-03T20:55:33Z ----------------------------------------------------------------

Done!

juanitorduz commented on 2024-06-05T12:45:09Z ----------------------------------------------------------------

I think the title is still missing!

wd60622 commented on 2024-06-08T06:45:22Z ----------------------------------------------------------------

+1

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-27T09:55:14Z ----------------------------------------------------------------

Can we have a 2 x 2 grid so that the plots look bigger?

wd60622 commented on 2024-05-29T12:33:29Z ----------------------------------------------------------------

+1

Like this plot! and really like the colors!

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-27T09:55:14Z ----------------------------------------------------------------

As above, thy do we have higher HDIs?