-
Notifications
You must be signed in to change notification settings - Fork 220
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
Add Weibull CDF and PDF Adstock Transformations #499
Conversation
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.
Nice work!
I left a couple of comments, no major issue
pymc_marketing/mmm/transformers.py
Outdated
k=1, | ||
l_max: int = 12, | ||
axis: int = 0, | ||
type: WeibullType = WeibullType.PDF, |
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.
I would use string or a boolean variable so users don't have to import an object to be able to parametrize the function
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.
+1
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.
We have a similar interface for batched_convolution too. Maybe make it type: WeibullType | str = WeibullType.PDF
?
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.
It will only every be pdf or not pdf (cdf), right? Think the boolean simplifies it heavily
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.
Just small comment on the docstring arg type
But other than that, looks good!
pymc_marketing/mmm/transformers.py
Outdated
Shape parameter of the Weibull distribution. Must be positive. | ||
l_max : int, by default 12 | ||
Maximum duration of carryover effect. | ||
type : WeibullType, by default WeibullType.PDF |
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.
Maybe good to add string to match with the type hint
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.
+1
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #499 +/- ##
==========================================
+ Coverage 91.24% 91.32% +0.08%
==========================================
Files 21 21
Lines 2044 2063 +19
==========================================
+ Hits 1865 1884 +19
Misses 179 179 ☔ View full report in Codecov by Sentry. |
Description
This pull request add the
weibull_adstock function
, providing support for both Weibull CDF and PDF adstock transformations.Related Issue
Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--499.org.readthedocs.build/en/499/