-
Notifications
You must be signed in to change notification settings - Fork 252
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
Fix tanh component #1303
Fix tanh component #1303
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1303 +/- ##
=======================================
Coverage 95.13% 95.13%
=======================================
Files 44 44
Lines 4629 4629
=======================================
Hits 4404 4404
Misses 225 225 ☔ View full report in Codecov by Sentry. |
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.
Can you remove beta default prior and beta from function signature
We should probably revert this, At glance I can see how it may appear we were multiplying twice, and the documentation didn't help explain why This is a fantastic way to state the response* function because the coefficients have very straightforward interpretations:
Before the PR got merged, you would achieve this parameterization by creating the saturation like: my_response = TanhSaturation(
priors={
"beta": Prior(...), # Business informed prior for ROAS at spend 0. Marketeers know this number
"b": Prior(...), # Business informed prior for maximum reachable eyeballs/effective dollars.
"c": 1,
}
) I find this parameterization HIGHLY superior to the form which is now—after this PR got merged—the only supported option in b still has the same interpretation. But what does c mean? @ferrine defined it as the "Initial cost per user. Must be non-zero." 3 YEARS AGO. This seems to originate from the Hello Fresh project, and is IMO not the most generally useful way to look at response/saturation. For small Yeah, so I hope I'm making sense here. IMO this inadvertedly PR broke important functionality. *: let's use the word response rather than saturation because that is in fact what this function is: it maps spend (or other similar driver unit) to a revenue (or other similar KPI) response. |
Shall we revert then? Since this PR, the user can pass priors to that are constants at initialization. Ie: sat = TanhSaturation(priors={"c": 1}) Other priors stay defaults and we can have the general form with easy support for alternative |
Yeah either revert for immediate effect, or make a new PR for the issue I created #1348. IMO there are some consistency issues with the saturation components that we can solve quickly. With @wd60622 and @juanitorduz 's blessings I can get to it right away. |
Can you create a separate issue to state the consistency issues. Lets iterate through small PRs vs single large changes |
OK I understand, you prefer to make one PR to revert this PR, and then another PR to fix the consistency issues. Even though the revert-PR could actually be skipped entirely as the consistency-PR would also revert this PR? |
You have to define consistency issue. That is my issue with this. Will it touch every saturation function making it have large effect? Edit: read the other issue that was linked above. Lets continue there. I am overall for smaller PRs with less drastic changes |
OK roger. |
Originally, there were three priors to parametrize something that has 2 degrees of freedom. That really was a bug, and should be fixed. I guess we can keep three parameters, and make sure users specify exactly two of those, and set the third to None, that would be fine as well, and give more flexibility. About your description ob b:
That's not really true. By definition there is no maximum, as tanh is strictly monotone. But I think we can easily fix this: "b is the value for x where we reach In the other parametrization, beta isn't just approximately 1/c, they are exactly the same thing, just the inverse: One is the initial (ie at zero) return on investment, the other the inverse return on investment (so the cost per user). So the two parametrizations differ in two ways:
There would be a third option by the way: spending value where the return of investment drops to half of the initial value (or equivalently the cost per user doubles). I like that one because it also still works if the response curve doesn't converge. I think it is perfectly fine if we support several options, but we should make sure that users only specify two of them. |
Fixes #1302
📚 Documentation preview 📚: https://pymc-marketing--1303.org.readthedocs.build/en/1303/