-
Notifications
You must be signed in to change notification settings - Fork 324
MMM Class Claude Sonnet 4.5 Refactor #1974
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1974 +/- ##
==========================================
+ Coverage 93.41% 93.83% +0.42%
==========================================
Files 67 67
Lines 8521 8585 +64
==========================================
+ Hits 7960 8056 +96
+ Misses 561 529 -32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the mmm.py
file to improve code readability and maintainability while preserving all function signatures and docstrings. The refactoring focuses on extracting repeated logic into helper methods, simplifying complex methods, and adding named constants to replace magic numbers.
Key changes include:
- Added constants for default values (HDI probability, quantiles, etc.)
- Extracted scaling logic into reusable helper methods
- Broke down the large
build_model()
method into focused helper methods - Simplified data preparation logic in
_data_setter()
and related methods
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
pymc_marketing/mmm/mmm.py:1
- Using union syntax
float | np.floating
requires Python 3.10+. Consider usingUnion[float, np.floating]
from typing module for broader compatibility or ensure minimum Python version requirement is documented.
# Copyright 2022 - 2025 The PyMC Labs Developers
tests/mmm/test_mmm.py:1
- Using union syntax
pd.Series | np.ndarray
requires Python 3.10+. Consider usingUnion[pd.Series, np.ndarray]
from typing module for broader compatibility.
# Copyright 2022 - 2025 The PyMC Labs Developers
tests/mmm/test_mmm.py:1
- Using union syntax
pd.Series | np.ndarray
requires Python 3.10+. Consider usingUnion[pd.Series, np.ndarray]
from typing module for broader compatibility.
# Copyright 2022 - 2025 The PyMC Labs Developers
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
3c651a0
to
8f86114
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
pymc_marketing/mmm/mmm.py:1
- Using
float | np.floating
union syntax requires Python 3.10+. Consider usingUnion[float, np.floating]
for broader compatibility or verify minimum Python version requirements.
# Copyright 2022 - 2025 The PyMC Labs Developers
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You might want to explore having it write a spec first: https://www.linkedin.com/posts/twiecki_advanced-context-engineering-for-agents-activity-7379009225378930688-T5g3?utm_source=share&utm_medium=member_desktop&rcm=ACoAAAlPjQsB9khSgXTWTM2phblua2XcZIyWpxY |
Indeed! I'm exploring, thanks for the reference 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
I'll review during the week! |
claude refactor mypy add tests tests optimization add edge cases tests rerun mmm example nb and compare results more tests
b6a88b6
to
ecd3204
Compare
looks like mu_effects 😁 Is there a timeline to deprecate this class? Is refactoring just to try out this tool? |
I second Will's proposal to deprecate this class as a whole. Should we mark 0.18.0 as the very last release with this around? |
The refactoring is a proof of concept on how we could improve the code on iterations with AI. Still, I suggest we merge it even if we will depreciate as the code is much better written now. So the review does not need to be so detailed as older tests pass and the api and function signatures are the same. |
I think we should start adding depreciation warnings and start porting all the notebook examples to the new api. I will create an issue with tasks. |
When do we release 1.0? |
Once we finally move the MMM Multidimensional completely (If I have to guess is around 0.19 or 0.20.0) |
There are also CLV API changes I want to make which could probably be done in the same releases. This will bring some additional functionality to MMM because the redundant CLV internals can be consolidated into the |
Prompt:
Think about the plan and then execute.
Refactoring Plan
After analyzing the code, I've identified these areas for improvement:
build_model
,_data_setter
,sample_posterior_predictive
Let me execute this refactoring:
[8 tools called]
Now let me check for any linting errors:
[4 tools called]
Let me now simplify other helper methods and add a summary of the refactoring work done:
[1 tool called]
Perfect! Let me create a summary of the refactoring work completed:
Summary of Refactoring
I've successfully refactored the
mmm.py
file to improve readability and maintainability while preserving all function signatures and docstrings. Here's what was done:1. Added Constants (Lines 67-72)
DEFAULT_HDI_PROB = 0.94
DEFAULT_LOWER_QUANTILE = 0.025
DEFAULT_UPPER_QUANTILE = 0.975
DEFAULT_QUANTILE = 0.5
DEFAULT_N_PERCENTILES = 30
2. Simplified Scaling Logic (Lines 404-454)
_compute_scale_for_data()
helper method to eliminate code duplication_compute_scales()
to use the helper method3. Extracted Model Building Components (Lines 527-775)
_create_scaled_data_variables()
- centralizes data scaling setup_build_intercept()
- handles time-varying and static intercept creation_build_channel_contribution()
- handles time-varying and static channel contributions_build_control_contribution()
- handles control variables_build_yearly_seasonality_contribution()
- handles seasonality_add_original_scale_deterministics()
- adds original scale variables4. Simplified
build_model()
Method (Lines 777-908)5. Refactored Data Setting Logic (Lines 1070-1219)
_has_new_scaling()
- checks scaling approach_prepare_channel_data()
- prepares channel data_prepare_control_data()
- prepares control data_prepare_target_data()
- prepares target data_data_setter()
from complex conditionals to clear, modular calls6. Improved
sample_posterior_predictive()
Method (Lines 2349-2499)_transform_to_original_scale_new()
- handles new scaling approach_transform_to_original_scale_legacy()
- handles legacy transformer approach7. Refactored Synthetic Dataset Creation (Lines 2632-2790)
_validate_controls_for_synth_dataset()
- validates controls_generate_future_dates()
- generates dates based on granularity_create_synth_dataset()
using helper methods8. Simplified Plotting Methods (Lines 1611-1776)
_get_intercept_for_plot()
- extracts intercept preparation_get_target_for_plot()
- extracts target preparationplot_components_contributions()
to be more readableKey Benefits:
✅ No breaking changes - All public APIs preserved
✅ Better readability - Complex methods broken into logical pieces
✅ Reduced duplication - Common patterns extracted to helpers
✅ Improved maintainability - Easier to test and modify individual components
✅ Clearer intent - Helper method names document what code does
✅ Better error messages - More descriptive validation errors
The refactoring follows best practices:
📚 Documentation preview 📚: https://pymc-marketing--1974.org.readthedocs.build/en/1974/