-
Notifications
You must be signed in to change notification settings - Fork 228
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
Move fit_result
to model_builder.py
and remove redundancies from CLV and MMM
#1344
Move fit_result
to model_builder.py
and remove redundancies from CLV and MMM
#1344
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1344 +/- ##
==========================================
- Coverage 95.35% 95.34% -0.01%
==========================================
Files 47 47
Lines 4951 4943 -8
==========================================
- Hits 4721 4713 -8
Misses 230 230 ☔ View full report in Codecov by Sentry. |
@wd60622 Created a draft PR. Let me know if these changes look good to you for this issue. |
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.
Looking good in package. Could you move any related tests into the model_builder test suite now. For instance, if there are checks for warnings in clv tests, just move to model_builder now and remove from clv
Thanks. Let me do that and also run all the unit tests once. Let me get back in a bit |
EDIT - Fixing the failing checks @wd60622 Did move two tests from CLV TL;DR - I think the PR is okay and the errors are local env specific. Let me know if you want me to move any other test. I don't think other tests should be moved but I may be wrong. |
I am fixing the fixtures then the tests should pass |
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 for making the change @sreekailash
Description
Have made the following changes:
fit_result
similar to posterior in themodel_builder
since it is still used across many filesbasic.py
in CLVfit_result
from MMM and CLV baseSubmitting a draft PR and appreciate any feedback.
Related Issue
fit_result
property into ModelBuilder #1333Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--1344.org.readthedocs.build/en/1344/