Make optimization based on the entire posterior and not on the marginal mean parameters. #1151

cetagostini · 2024-11-02T15:58:59Z

Description

Structural change on how responses are computed in the optimizer, we are now using the entire "posterior" of the model, and additionally we are creating a new notebook where we can encode certain risk levels.

Related Issue

Closes #
Related to #

Checklist

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

Modules affected

MMM
CLV

Type of change

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

review-notebook-app · 2024-11-02T15:59:05Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2024-11-02T16:25:36Z

Codecov Report

Attention: Patch coverage is 87.34177% with 20 lines in your changes missing coverage. Please review.

Project coverage is 95.12%. Comparing base (1d89f66) to head (915ff64).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pymc_marketing/mmm/utility.py	86.02%	13 Missing ⚠️
pymc_marketing/mmm/mmm.py	70.00%	6 Missing ⚠️
pymc_marketing/mmm/budget_optimizer.py	97.77%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1151      +/-   ##
==========================================
- Coverage   95.64%   95.12%   -0.53%     
==========================================
  Files          39       40       +1     
  Lines        4091     4228     +137     
==========================================
+ Hits         3913     4022     +109     
- Misses        178      206      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pymc_marketing/mmm/mmm.py

pymc_marketing/mmm/budget_optimizer.py

pymc_marketing/mmm/mmm.py

pymc_marketing/mmm/risk_assessment.py

tests/mmm/test_risk_assessment.py

pymc_marketing/mmm/budget_optimizer.py

pymc_marketing/mmm/mmm.py

review-notebook-app · 2024-11-12T20:06:44Z

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-11-12T20:06:43Z
----------------------------------------------------------------

Line #3.    plt.title("Response Distribution at 95% Confidence Level");

We should use the term HDI (highest density interval)

review-notebook-app · 2024-11-12T20:06:44Z

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-11-12T20:06:44Z
----------------------------------------------------------------

Same comment bout the HDI in the title

It would be also nice to have them both plotten in the same figure to compare them :)

review-notebook-app · 2024-11-12T20:06:45Z

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-11-12T20:06:45Z
----------------------------------------------------------------

Same comment about the title

review-notebook-app · 2024-11-12T20:06:47Z

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-11-12T20:06:46Z
----------------------------------------------------------------

Did anything actually change in the example notebook or can we simply remove the changes ?

Co-Authored-By: Will Dean <57733339+wd60622@users.noreply.github.com>

cetagostini · 2024-11-21T11:23:56Z

All notebooks update to use the new optimization method.

cetagostini · 2024-11-27T19:35:34Z

Ready for final review, everything should be done.

Cc: @juanitorduz @cluhmann

juanitorduz · 2024-11-28T15:36:10Z

Thank @cetagostini ! I will review this one soon (I promise).

One thing you could improve while we do it is making sure we test all functions. For example, I see from the coverage report we do not have a test for the function (plot_allocated_contribution_by_channel). We need at least one test before we merge (If there is a test and I missed then it is fine :) )

review-notebook-app · 2024-11-29T18:59:17Z

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-11-29T18:59:16Z
----------------------------------------------------------------

possitive is a typo, we shoudl revert this

review-notebook-app · 2024-11-29T18:59:18Z

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-11-29T18:59:17Z
----------------------------------------------------------------

We shoudl revert this as we want to use the Prior class

review-notebook-app · 2024-11-29T18:59:19Z

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-11-29T18:59:18Z
----------------------------------------------------------------

wrong correction

juanitorduz

@cetagostini can we revert the examples in the example notebook? This seems like an old version and has many errors and typos 🙏 (can you please check the other notebooks in detail as well to see iv we have the same issue?)

juanitorduz

I tried to give a detailed look and left some comments 🙏

juanitorduz · 2024-11-29T19:42:11Z

pymc_marketing/mmm/budget_optimizer.py

-                "method": "SLSQP",
-                "options": {"ftol": 1e-9, "maxiter": 1_000},
-            }
+            minimize_kwargs = self.DEFAULT_MINIMIZE_KWARGS


Can we consider #1193 here?

Like it, will apply it.

pymc_marketing/mmm/utility.py

juanitorduz · 2024-11-29T19:43:58Z

pymc_marketing/mmm/utility.py

+
+import pytensor.tensor as pt
+
+UtilityFunction = Callable[[pt.TensorVariable, pt.TensorVariable], float]


Shall we call this UtilityFunctionType? Otherwise it could be interpreted as a class

Yes, we can do that!

juanitorduz · 2024-11-29T19:45:38Z

pymc_marketing/mmm/utility.py

+def average_response(
+    samples: pt.TensorVariable, budgets: pt.TensorVariable
+) -> pt.TensorVariable:
+    """Compute the average response of the posterior predictive distribution."""
+    return pt.mean(samples)


Do we really need this function? Note we are not using the budget variable at all, right?

We need the same signature in all functions in order to make compatible an scalable. I had an implementation that was more directed per function but was taking a few flexibility from users, with @wd60622 we arrive to this conclusion with makes everything more flexible, the only inconvenience is we need samples and budgets across all, even when we don't use them.

juanitorduz · 2024-11-29T19:47:26Z

pymc_marketing/mmm/utility.py

+        raise ValueError("Confidence level must be between 0 and 1.")
+
+    def _tail_distance(
+        samples: pt.TensorVariable, budgets: pt.TensorVariable


we are not using the budgets variable, so we should remove it right?

Replied before!

juanitorduz · 2024-11-29T19:51:32Z

pymc_marketing/mmm/utility.py

+    ----------
+    .. [1] Rockafellar, R.T., & Uryasev, S. (2000). Optimization of Conditional Value-at-Risk.
+    """
+    if not 0 < confidence_level < 1:


any problems if is exactly zero or one?

juanitorduz · 2024-11-29T19:52:23Z

pymc_marketing/mmm/utility.py

+
+    Parameters
+    ----------
+    confidence_level : float, optional


We should add "confidence level must be between 0 and 1."

juanitorduz · 2024-11-29T19:54:18Z

tests/mmm/test_budget_optimizer.py

-                "channel_1": {
-                    "adstock_params": {"alpha": 0.5},
-                    "saturation_params": {"lam": 10, "beta": 0.5},
+                "saturation_params": {


should we still test the scalar case?

What do you mean here? What scalar case?

juanitorduz · 2024-11-29T19:54:45Z

tests/mmm/test_utility.py

+
+rng: np.random.Generator = np.random.default_rng(seed=42)
+
+EXPECTED_RESULTS = {


where are these values coming from?

I ran a notebook to check what would be the results for each function given the parameters, this test validates they return those values every time, meaning, function behavior its not changing and return consistent results.

juanitorduz · 2024-11-29T19:55:23Z

pymc_marketing/mmm/utility.py

+    UtilityFunction
+        A function that calculates the tail distance metric given samples and budgets.
+    """
+    if not 0 < confidence_level < 1:


We should add a test fro these ValueErrors

Alternatively, we could use @validate_call from pydantic as in https://github.com/pymc-labs/pymc-marketing/blob/main/pymc_marketing/mmm/mmm.py#L74

juanitorduz · 2024-11-29T20:02:42Z

Important Comments:

Before we merge, we need to make sure the changes in the notebooks are meaningful. For example, the MMM Example version seems old as it reverts typo corrections and does not use the Prior class. We should only change the notebooks relevant to this PR.
Can you please re-run the MMM case study and expand on the budget allocation section a bit to showcase the gains of this new approach and how to use it in practice?

Thank you for this great and ambitious PR, @cetagostini ! These suggestions are to make sure we deliver a great solution 💪 🙇

cetagostini · 2024-11-29T21:12:31Z

Important Comments:

Before we merge, we need to make sure the changes in the notebooks are meaningful. For example, the MMM Example version seems old as it reverts typo corrections and does not use the Prior class. We should only change the notebooks relevant to this PR.
Can you please re-run the MMM case study and expand on the budget allocation section a bit to showcase the gains of this new approach and how to use it in practice?

Thank you for this great and ambitious PR, @cetagostini ! These suggestions are to make sure we deliver a great solution 💪 🙇

Thank you for the detailed review, I need some feedback on a few comments, I'll proceed with the rest 🙌🏻

PS: Sorry for the MMM example, I got issues with the notebook and maybe the rebase revert those, let me bring back the one from main @juanitorduz

juanitorduz · 2024-11-29T22:17:25Z

Thanks @cetagostini ! Git and notebooks is a pain!👌🙇

(btw cool pytensor stuff... I am learning quite a lot from these risks metrics!)

github-actions bot added docs Improvements or additions to documentation MMM tests labels Nov 2, 2024

cetagostini requested review from wd60622 and juanitorduz November 2, 2024 16:01

cetagostini requested a review from cluhmann November 2, 2024 16:28

wd60622 added the enhancement New feature or request label Nov 2, 2024

wd60622 reviewed Nov 2, 2024

View reviewed changes

pymc_marketing/mmm/mmm.py Outdated Show resolved Hide resolved

pymc_marketing/mmm/budget_optimizer.py Outdated Show resolved Hide resolved

pymc_marketing/mmm/budget_optimizer.py Outdated Show resolved Hide resolved

wd60622 mentioned this pull request Nov 4, 2024

allocate_budget_to_maximize_response() doc string could be clearer #1161

Open

cetagostini requested a review from wd60622 November 12, 2024 11:09