Add FrequencyReachAdditiveEffect#1968
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1968 +/- ##
==========================================
- Coverage 92.51% 91.49% -1.03%
==========================================
Files 68 70 +2
Lines 9398 9536 +138
==========================================
+ Hits 8695 8725 +30
- Misses 703 811 +108 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Shall we change additive_effect.py into additive_effects/ submodule and has this as a file?
There was a problem hiding this comment.
All can added to __init__ so that the import is kept
There was a problem hiding this comment.
I am happy to move things around, but for me the main question of this PR is:
- Should we somehow modify
SaturationTransformationto offer strong separation between saturation asymptote and lambda, c, slope, etc?
|
Amazing! |
TeemuSailynoja
left a comment
TeemuSailynoja
left a comment
There was a problem hiding this comment.
The order of adstock and saturation is reversed in the documentation.
Should we reference the source paper instead of Meridian documentation?
| saturation : SaturationTransformation | ||
| Saturation transformation applied after adstock. | ||
| adstock : AdstockTransformation | ||
| Adstock transformation applied first to the effective exposure signal. |
There was a problem hiding this comment.
Ordering here is reversed in relation to the code. Code is correct – first saturation, then adstock.
| * If channels present in the DataFrame are not a subset of the MMM channel | ||
| coordinate, a ValueError is raised. | ||
|
|
There was a problem hiding this comment.
Why is this required? I would assume the opposite. Isn't reach + frequency an alternative to using impressions or spend, and having both in the model would effectively double count the contribution of the involved channels?
The estimated contributions are also separate from the channel_contribution deterministic in the parent MMM, so this could lead to confusion.
| beta = pm.Normal( | ||
| f"{self.prefix}_beta", | ||
| mu=0.0, | ||
| sigma=1.0, | ||
| dims=(self.channel_coord_name,), | ||
| ) |
There was a problem hiding this comment.
perhaps expose to user as an optional argument of the Prior class, and have this as a default value.
5 participants
Description
Add Frequency/Reach support via the
MuEffectinterface.Similar, but slightly different (see below) to the implementation described here
Notes:
1) The formulation is not strictly equivalent. Notice that ourfrequency_sat: Tensorvariablealready applies the multiplicativeBetacoefficient. If we want a fully equivalent implementation we need to call , but then we cannot rely on.applyto do dim_handling.Introduced
HillShapeSaturationto make the formulation equivalentThis is a very drafty feature. No support for optimization whatsoever.
Related Issue
Checklist
pre-commit.ci autofixto auto-fix.📚 Documentation preview 📚: https://pymc-marketing--1968.org.readthedocs.build/en/1968/