-
Notifications
You must be signed in to change notification settings - Fork 324
Add FrequencyReachAdditiveEffect
#1968
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 #1968 +/- ##
==========================================
- Coverage 92.18% 91.12% -1.07%
==========================================
Files 67 69 +2
Lines 8147 8424 +277
==========================================
+ Hits 7510 7676 +166
- Misses 637 748 +111 ☔ 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.
Shall we change additive_effect.py into additive_effects/ submodule and has this as a file?
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.
All can added to __init__
so that the import is kept
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 like this idea!
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 am happy to move things around, but for me the main question of this PR is:
- Should we somehow modify
SaturationTransformation
to offer strong separation between saturation asymptote and lambda, c, slope, etc?
Amazing! |
Description
Add Frequency/Reach support via the
MuEffect
interface.Similar, but slightly different (see below) to the implementation described here
Notes:
1) The formulation is not strictly equivalent. Notice that ourfrequency_sat: Tensorvariable
already applies the multiplicativeBeta
coefficient. If we want a fully equivalent implementation we need to call , but then we cannot rely on.apply
to do dim_handling.Introduced
HillShapeSaturation
to make the formulation equivalentThis is a very drafty feature. No support for optimization whatsoever.
Related Issue
Checklist
pre-commit.ci autofix
to auto-fix.📚 Documentation preview 📚: https://pymc-marketing--1968.org.readthedocs.build/en/1968/