Skip to content

Conversation

@cetagostini
Copy link
Contributor

@cetagostini cetagostini commented Jul 16, 2025

Description

Marked the budget optimization method as deprecated and replaced its implementation with a NotImplementedError. Users are advised to migrate to the Multidimensal.MMM class.

Related Issue

  • Closes #
  • Related to #

Checklist


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

Marked the budget optimization method as deprecated and replaced its implementation with a NotImplementedError. Users are advised to migrate to the `Multidimensal.MMM` class.
@github-actions github-actions bot added the MMM label Jul 16, 2025
@codecov
Copy link

codecov bot commented Jul 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@492930a). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1832   +/-   ##
=======================================
  Coverage        ?   92.28%           
=======================================
  Files           ?       64           
  Lines           ?     7430           
  Branches        ?        0           
=======================================
  Hits            ?     6857           
  Misses          ?      573           
  Partials        ?        0           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

budget_bounds=budget_bounds,
callback=callback,
**minimize_kwargs,
raise NotImplementedError(
Copy link
Contributor

Choose a reason for hiding this comment

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

can we warn instead of raise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think better raise, only because the current method even if it works will return incorrect results. No sense to keep it, but what do you think?

Comment on lines 2339 to 2341
"""Optimize the given budget based on the specified utility function over a specified time period (DEPRECATED).
This function optimizes the allocation of a given budget across different channels
DEPRECATED: This function optimizes the allocation of a given budget across different channels
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a specific way to call out in numpydocs

https://numpydoc.readthedocs.io/en/latest/format.html#deprecation-warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do this!

@cetagostini
Copy link
Contributor Author

This complements #1849 deprecating method, meanwhile 1849 remove and update notebooks and dependencies.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the docs Improvements or additions to documentation label Jul 26, 2025
@github-actions github-actions bot added the tests label Jul 26, 2025
@cetagostini cetagostini added bug Something isn't working maintenance labels Jul 26, 2025
@cetagostini cetagostini changed the title Deprecate and disable budget optimization method Deprecate and disable budget optimization method & Updating notebook Jul 26, 2025
@juanitorduz
Copy link
Collaborator

@cetagostini i think its better to warn on the deprecation and remove in in 0.17.0

People are running these models in production and this is a key method. We will remove it for sure, but I think we need to do it in two steps.

Thoughts?

@juanitorduz
Copy link
Collaborator

In the meantime… while warning. Can we somehow use the optimizer from the mmm multi clas I the old mmm class?

@PabloRoque
Copy link
Contributor

@cetagostini i think its better to warn on the deprecation and remove in in 0.17.0

People are running these models in production and this is a key method. We will remove it for sure, but I think we need to do it in two steps.

Thoughts?

Users can always pin to 0.15.1. Maybe raise the error and tell them to use 0.15.1?

@cetagostini
Copy link
Contributor Author

In the meantime… while warning. Can we somehow use the optimizer from the mmm multi clas I the old mmm class?

So, I could work on a PR to modify MMM base model to work as multidimensional, and be okay with the optimizer again. Nevertheless, probably I agree with Pablo that users who wants to continue with MMM base can always pin 15.1. But I follow your lead here @juanitorduz

You dictate the course and I'll follow capitan!

@juanitorduz
Copy link
Collaborator

So, I could work on a PR to modify MMM base model to work as multidimensional, and be okay with the optimizer again.

How much work would this be for you ? :)

@cetagostini
Copy link
Contributor Author

So, I could work on a PR to modify MMM base model to work as multidimensional, and be okay with the optimizer again.

How much work would this be for you ? :)

Maybe another day or two? (Meaning maybe like 5 days in OSS days 😅) Should I open the PR only adjusting scaling method on base MMM?

@juanitorduz
Copy link
Collaborator

I mean, I think we can wait a week (or two), there is really no rush. It would be great to modify MMM base model to work as multidimensional, if possible, in view that I know many people looking into the end-to-end notebook: https://www.pymc-marketing.io/en/latest/notebooks/mmm/mmm_case_study.html. Also, the MMM multidim still does not fully support the HSGP (see #1432). Hence, if we remove the optimizer, users won't be able to have a single model to fit and optimize.

Hence, I suggest we modify MMM base model to work as multidimensional. No rush. Let me know if I can help.

Does it make sense?

@cetagostini
Copy link
Contributor Author

Okay, I'll move this to draft, and will try to crack a PR for that during the next few days!

@cetagostini cetagostini marked this pull request as draft July 29, 2025 17:46
@juanitorduz
Copy link
Collaborator

Okay, I'll move this to draft, and will try to crack a PR for that during the next few days!

Thank you @cetagostini ❤️ . I will buy you a beer in September (PyData Berlin) as a reward for this fantastic work with the optimizer! #hero

@juanitorduz juanitorduz added this to the 0.16.0 milestone Jul 30, 2025
@cetagostini
Copy link
Contributor Author

I need to get #1862 merge before continue here.

@juanitorduz
Copy link
Collaborator

I need to get #1862 merge before continue here.

I just merged #1862 ! Thanks 🚀

@cetagostini cetagostini marked this pull request as ready for review August 10, 2025 19:35
@cetagostini
Copy link
Contributor Author

This looks ready, but the notebook get issue. I feel it's related to the nc loaded from my env, can you give me a hand here? and push the one you have @juanitorduz ? That should be all.

Thank you!

@juanitorduz
Copy link
Collaborator

@carlosagostini can you please comment out the line mmm.save("multidimensional_model.nc") in the https://www.pymc-marketing.io/en/latest/notebooks/mmm/mmm_multidimensional_example.html#save-model notebook? I think this might be causing the problem as the model is saved every time the notebook tests are run (we had a similar issue in the past with the normal mmm notebook)

@juanitorduz
Copy link
Collaborator

Thank you @carlosagostini !

@cetagostini cetagostini merged commit 9602e91 into main Aug 10, 2025
31 of 32 checks passed
@cetagostini cetagostini deleted the cetagostini/deprecating_optimizer_for_mmm branch August 10, 2025 20:21
@rdevvvv
Copy link

rdevvvv commented Nov 21, 2025

Hello! Thanks for the good work. I am currently following: https://www.pymc-marketing.io/en/0.11.0/notebooks/mmm/mmm_case_study.html#media-deep-dive and I was wondering what the next course of action was to optimize budget. My MMM is not multidimensionnal so I don't understand. Do I rewrite my code to use the multidimensional class but with just one dimension? It seems inefficient and I don't know if I can even do that. Thanks,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working docs Improvements or additions to documentation maintenance MMM tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants