-
Notifications
You must be signed in to change notification settings - Fork 324
Refactor ModelBuilder
into smaller classes
#1870
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
Refactor ModelBuilder
into smaller classes
#1870
Conversation
@williambdean do you still think the check parameter for |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1870 +/- ##
==========================================
+ Coverage 91.89% 92.15% +0.26%
==========================================
Files 64 64
Lines 7577 7587 +10
==========================================
+ Hits 6963 6992 +29
+ Misses 614 595 -19 ☔ 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.
Looking through. Not many objections. What else is needed here?
Just your thoughts on including the check parameter for the Testing coverage is 99% locally; not sure why the CodeCov bot is always lower than the |
From my side looks great! @williambdean, anything we are missing? |
As there are no comments I suggest we merge this on :) Amazing work @ColtAllen ! |
It was a biggie, hadn't found the time, sorry 🙈 |
All good! Thank you @PabloRoque 💪 |
Description
@williambdean also deserves contributor credit for starting this PR.
ModelBuilder
was originally developed to build models with an API very similar to the supervised learning models inscikit-learn
. This works fine for MMMs, but creates considerable tech debt when different APIs are required. This PR is the first of several to clean up accumulated tech debt by dividingModelBuilder
into three separate classes:Click to toggle UML diagram
The CLV models still inherit from a stripped-down
ModelBuilder
class, but all other models now inherit fromRegressionModelBuilder
. If the MNLogit and Nested Logit models are modified to inherit fromModelBuilder
instead, the internals of both could be cleaned up quite a bit.Related Issue
load
withModelBuilder
classmethod #1380Checklist
pre-commit.ci autofix
to auto-fix.📚 Documentation preview 📚: https://pymc-marketing--1870.org.readthedocs.build/en/1870/