Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Time slice cross validation MMM Notebook #971

Merged
merged 17 commits into from
Aug 29, 2024
Merged

Time slice cross validation MMM Notebook #971

merged 17 commits into from
Aug 29, 2024

Conversation

juanitorduz
Copy link
Collaborator

@juanitorduz juanitorduz commented Aug 24, 2024 •
edited by github-actions bot
Loading

Closes #956

📚 Documentation preview 📚: https://pymc-marketing--971.org.readthedocs.build/en/971/

@juanitorduz juanitorduz self-assigned this Aug 24, 2024
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@juanitorduz juanitorduz marked this pull request as draft August 24, 2024 08:16
@juanitorduz juanitorduz added enhancement New feature or request MMM tests labels Aug 24, 2024
@juanitorduz juanitorduz added this to the 0.9.0 milestone Aug 24, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.62%. Comparing base (28b75c3) to head (d70e77e).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #971      +/-   ##
==========================================
+ Coverage   95.59%   95.62%   +0.02%     
==========================================
  Files          35       36       +1     
  Lines        3499     3517      +18     
==========================================
+ Hits         3345     3363      +18     
  Misses        154      154

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@juanitorduz juanitorduz marked this pull request as ready for review August 25, 2024 09:08
wd60622
wd60622 reviewed Aug 29, 2024
View reviewed changes
Copy link
Contributor

@wd60622 wd60622 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks really good. Some questions. Looking more into the notebook now

pymc_marketing/metrics.py Show resolved Hide resolved
pymc_marketing/metrics.py Show resolved Hide resolved
pymc_marketing/metrics.py Show resolved Hide resolved
@review-notebook-app Review Notebook App
Copy link

review-notebook-app bot commented Aug 29, 2024 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

wd60622 commented on 2024-08-29T03:35:59Z
----------------------------------------------------------------

"It t can be" -> "It can be"

Also, are people suppose to use the per_observation_crsp function as well? If so, could we give an example of that and give a description of what each value would signify


juanitorduz commented on 2024-08-29T07:27:24Z
----------------------------------------------------------------

I think usually people woill look into the average (crps)

@review-notebook-app Review Notebook App
Copy link

review-notebook-app bot commented Aug 29, 2024 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

wd60622 commented on 2024-08-29T03:35:59Z
----------------------------------------------------------------

Line #6.                apply_sklearn_transformer_across_dim(

This is the only way to do this currently? We don't have it wrapped up? :(

@review-notebook-app Review Notebook App
Copy link

review-notebook-app bot commented Aug 29, 2024 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

wd60622 commented on 2024-08-29T03:36:00Z
----------------------------------------------------------------

"Event hough" -> "Even though"


juanitorduz commented on 2024-08-29T07:28:18Z
----------------------------------------------------------------

Thanks!

@juanitorduz Review Notebook App
Copy link
Collaborator Author

I think usually people woill look into the average (crps)

View entire conversation on ReviewNB

@juanitorduz Review Notebook App
Copy link
Collaborator Author

Thanks!

View entire conversation on ReviewNB

wd60622
wd60622 approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@wd60622 wd60622 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks for the adjustments

@juanitorduz juanitorduz merged commit cafa3f8 into main Aug 29, 2024
12 checks passed
@juanitorduz juanitorduz deleted the time-slice-cross-validation branch August 29, 2024 13:27
@juanitorduz
Copy link
Collaborator Author

Thank you @wd60622 for the review and feedback 🙏

radiokosmos pushed a commit to radiokosmos/pymc-marketing that referenced this pull request Sep 3, 2024
@juanitorduz @radiokosmos
Time slice cross validation MMM Notebook (pymc-labs#971)
a9450df 
* nb init

* add param stability

* crps init

* add crps to package

* make mypy happy

* clean

* relax req

* update path

* add crps test

* first iteration

* improvements

* improve tests

* improve tests

* improve references

* feedback 1

* add examples
twiecki pushed a commit that referenced this pull request Sep 10, 2024
@juanitorduz @twiecki
Time slice cross validation MMM Notebook (#971)
70f7104 
* nb init

* add param stability

* crps init

* add crps to package

* make mypy happy

* clean

* relax req

* update path

* add crps test

* first iteration

* improvements

* improve tests

* improve tests

* improve references

* feedback 1

* add examples
twiecki pushed a commit that referenced this pull request Sep 10, 2024
@juanitorduz @twiecki
Time slice cross validation MMM Notebook (#971)
f620bc8 
* nb init

* add param stability

* crps init

* add crps to package

* make mypy happy

* clean

* relax req

* update path

* add crps test

* first iteration

* improvements

* improve tests

* improve tests

* improve references

* feedback 1

* add examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wd60622 wd60622 wd60622 approved these changes

Assignees

@juanitorduz juanitorduz

Labels
enhancement New feature or request MMM tests
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.

Add time-slice cross validation notebook.
2 participants
@juanitorduz @wd60622