Create wrapper to make budget optimizer compatible with multidimensional class #1652
Conversation
Not working yet
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Plots are not working at the moment. I'll to work on them during tomorrow, the automatic dims selection is not properly doing the job! |
… of https://github.com/pymc-labs/pymc-marketing into cetagostini/multidimensional_compatible_with_optimizer
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1652 +/- ##
==========================================
- Coverage 93.39% 90.66% -2.73%
==========================================
Files 56 56
Lines 6329 6623 +294
==========================================
+ Hits 5911 6005 +94
- Misses 418 618 +200 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
View / edit / reply to this conversation on ReviewNB twiecki commented on 2025-04-27T05:23:46Z Line #1. optimizible_model = MultiDimensionalBudgetOptimizerWrapper( optimizable |
|
View / edit / reply to this conversation on ReviewNB twiecki commented on 2025-04-27T05:23:47Z A plot would round it out nicely. |
|
Typo solved, thanks! @twiecki plots added and working! |
Ricky always helping! Co-Authored-By: Ricardo Vieira <[email protected]>
|
View / edit / reply to this conversation on ReviewNB williambdean commented on 2025-04-29T12:59:44Z Can you add some context here and some references for the budget optimization process. Can be brief or reference some of the other documentation that we already have.
Also, can you go into what the budget of "10" means. Is that millions?
The other MMM class has these wrapped in some methods. Would it be difficult to reproduce with the same API for those that are coming from the other class and looking to use this functionality cetagostini commented on 2025-04-29T14:25:33Z Thats the idea, but I don't think its possible without change the budget_optimizer.py to support a more flexible implementation, which probably will break a few stuff. I share a private message about this. cetagostini commented on 2025-04-29T14:27:03Z We'll do it as soon as we consolidate, so the new class will integrate the wrapper logic. williambdean commented on 2025-04-29T21:06:32Z Okay. let's wait on the wrapper then. If you can add markdown above instead of the comment, I think that would look a bit better and more clear about the context of this cell cetagostini commented on 2025-04-30T20:20:22Z Not sure, I follow, what do you want to modify here? williambdean commented on 2025-05-01T01:15:56Z The markdown cell is just empty. There should be some type of transition from the previous out of sample section into the budget optimizer section. Some markdown should facilitate that switch |
|
View / edit / reply to this conversation on ReviewNB williambdean commented on 2025-04-29T12:59:45Z Line #1. # This objects is an xarray dataset with the allocation and posterior predictive responses Can we add this as markdown instead |
|
View / edit / reply to this conversation on ReviewNB williambdean commented on 2025-04-29T12:59:46Z Can you briefly explain the chart. Is this showing a comparison between what is being done and what is the optimal solution? Or is this just the optimal solution? |
|
View / edit / reply to this conversation on ReviewNB williambdean commented on 2025-04-29T12:59:46Z The comment here confuses me. Are we handling the scaling of the channel contributions for them? Or is that something that they will have to think about themselve? cetagostini commented on 2025-04-29T14:48:53Z I added a explanation, but only applies for custom models which are wrapper into the protocol. A random person could get the PyMC model into the protocol and then use the wrapper to optimize. But the plot will not be in original scale, and for that they can add some scaler to make it original scale. |
|
Thats the idea, but I don't think its possible without change the budget_optimizer.py to support a more flexible implementation, which probably will break a few stuff. I share a private message about this. View entire conversation on ReviewNB |
|
We'll do it as soon as we consolidate, so the new class will integrate the wrapper logic. View entire conversation on ReviewNB |
|
I added a explanation, but only applies for custom models which are wrapper into the protocol. A random person could get the PyMC model into the protocol and then use the wrapper to optimize. But the plot will not be in original scale, and for that they can add some scaler to make it original scale. View entire conversation on ReviewNB |
|
Okay. let's wait on the wrapper then. If you can add markdown above instead of the comment, I think that would look a bit better and more clear about the context of this cell View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB williambdean commented on 2025-04-29T21:10:48Z typo: wrappe -> wrapped
Let's link to the protocol class |
|
Not sure, I follow, what do you want to modify here? View entire conversation on ReviewNB |
|
The markdown cell is just empty. There should be some type of transition from the previous out of sample section into the budget optimizer section. Some markdown should facilitate that switch View entire conversation on ReviewNB |
Description
The new PR creates a wrapper which allows the optimizer to work on top of the multi-dimensional class. The only reason to not replace directly the code on the
optimizer.pyis to give us time to make the transition between the classes MMM and multidimensional MMM, and not break users current behavior which can be potentially an issue with the needed changes for the class.Related Issue
Checklist
pre-commit.ci autofixto auto-fix.📚 Documentation preview 📚: https://pymc-marketing--1652.org.readthedocs.build/en/1652/