-
Notifications
You must be signed in to change notification settings - Fork 192
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
Time Varying Media Contribution Notebook #778
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #778 +/- ##
=======================================
Coverage 93.86% 93.86%
=======================================
Files 28 28
Lines 2950 2950
=======================================
Hits 2769 2769
Misses 181 181 ☔ View full report in Codecov by Sentry. |
View / edit / reply to this conversation on ReviewNB wd60622 commented on 2024-06-23T19:13:13Z can use "\ |
View / edit / reply to this conversation on ReviewNB wd60622 commented on 2024-06-23T19:13:13Z Why not just show the graph? cetagostini commented on 2024-06-23T20:44:49Z Good idea! |
View / edit / reply to this conversation on ReviewNB wd60622 commented on 2024-06-23T19:13:14Z Does this mean the intercept is underestimated? cetagostini commented on 2024-06-23T20:44:23Z Indeed but a little, real intercept is juanitorduz commented on 2024-06-24T09:55:33Z Can you experiment with the |
Indeed but a little, real intercept is View entire conversation on ReviewNB |
Good idea! View entire conversation on ReviewNB |
Can you experiment with the View entire conversation on ReviewNB |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-06-24T11:20:49Z Can you take λ_t outside the sum to really show is independent from m? wd60622 commented on 2024-06-25T04:09:21Z +1 cetagostini commented on 2024-06-28T17:43:46Z Done! |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-06-24T14:56:17Z Could to please add a title and x/y axis labels :) cetagostini commented on 2024-06-28T17:52:15Z Done! |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-06-24T14:56:17Z canyou put these plots in same columns but different rows (plt.subplots(nrows=2, ncols=1) and use date x-labels? |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-06-24T14:56:18Z date x ranges and maybe also plt.subplots(nrows=2, ncols=1) ? cetagostini commented on 2024-06-28T17:56:50Z Done! |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-06-24T14:56:19Z Can we use black for the target variable color? Also x date range labels? |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-06-24T14:56:20Z Line #14. target_accept=0.98, Can we reduce this target accept to alt least 0.9 or 0.95 ? Otherwise this makes the model fit a bit concerning. cetagostini commented on 2024-06-28T17:58:50Z done! |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-06-24T14:56:20Z Line #4. target_accept=0.98, Here we should aim to reduce the target_accept value |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-06-24T14:56:21Z "great" with lowe case? |
@cetagostini, very cool notebook! I left some comments :) |
+1 View entire conversation on ReviewNB |
Done! View entire conversation on ReviewNB |
Done! View entire conversation on ReviewNB |
Done! View entire conversation on ReviewNB |
done! View entire conversation on ReviewNB |
pre-commit.ci autofix |
pre-commit.ci autofix |
pre-commit.ci autofix |
What do you think @cetagostini @cetagostini-wise |
View / edit / reply to this conversation on ReviewNB juanitorduz commented on 2024-07-01T09:28:16Z Can we use date_week as the x-axis variable instead of weeks? (and all plots below) |
pre-commit.ci autofix |
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.
Amazing
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.
Thanks @cetagostini !
* Time Varaying notebook * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * requested changes * quick change * Adding to index md * remove change mmm_budget_allocation_example.ipynb * quick correction * Small changes * Small title change --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Will Dean <[email protected]>
* Time Varaying notebook * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * requested changes * quick change * Adding to index md * remove change mmm_budget_allocation_example.ipynb * quick correction * Small changes * Small title change --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Will Dean <[email protected]>
Description
Related Issue
Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--778.org.readthedocs.build/en/778/