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

remove spend scaling step from budget optimizer #1070

Merged
merged 2 commits into from
Sep 28, 2024
Merged

Conversation

juanitorduz
Copy link
Collaborator

@juanitorduz juanitorduz commented Sep 27, 2024

When looking into #1044 I encountered the following issue: The budget to allocate in the example is of the order of ~ 1e6 and I was getting an uniform split

pd.Series(mmm.optimal_allocation_dict)

Direct Mail       1.972773e+06
Insert            1.972773e+06
Newspaper         1.972773e+06
Online Display    1.972773e+06
Radio             1.972773e+06
Social Media      1.972773e+06
TV                1.972773e+06
dtype: float64

Nevertheless the plot

fig, ax = mmm.plot_budget_allocation(samples=response, figsize=(12, 8))
fig.suptitle("Response vs spent per channel");

showed

image

which was values of the order ~ 1e12.

I talked with @carlosagostini and notice that with the latest changes we do not need to scale the spends.


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

@juanitorduz juanitorduz marked this pull request as draft September 27, 2024 15:14
Comment on lines +2246 to +2247
if original_scale:
channel_contributions *= self.get_target_transformer()["scaler"].scale_
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@carlosagostini, Is this still valid, or should we remove the scaling everywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Contributions still need to be scaled, but not the budget allocation, because the optimizer now returns it in original scale. So post processing is not necessary 🙌🏻

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok! Thanks! Si is this one good to go ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed!

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@juanitorduz
Copy link
Collaborator Author

In bc78e64 I re-ran the notebook. The results look as expected right?

@juanitorduz juanitorduz marked this pull request as ready for review September 27, 2024 15:29
@juanitorduz
Copy link
Collaborator Author

BTW: I changed the colours so that they can be synced with the user palettes (that is why we are using the "C0" and "C1") syntax

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 95.88%. Comparing base (d05c2d8) to head (bc78e64).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pymc_marketing/mmm/mmm.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1070      +/-   ##
==========================================
+ Coverage   95.85%   95.88%   +0.02%     
==========================================
  Files          39       39              
  Lines        3934     3933       -1     
==========================================
  Hits         3771     3771              
+ Misses        163      162       -1     
Flag Coverage Δ
95.88% <0.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@cetagostini cetagostini left a comment

Choose a reason for hiding this comment

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

Everything looks good here!

@juanitorduz juanitorduz merged commit da26dc8 into main Sep 28, 2024
11 of 12 checks passed
@juanitorduz juanitorduz deleted the rm-scaler-optimizer branch September 28, 2024 08:39
@juanitorduz juanitorduz added docs Improvements or additions to documentation MMM labels Sep 28, 2024
jsnyde0 pushed a commit to jsnyde0/pymc-marketing that referenced this pull request Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation MMM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants