-
Notifications
You must be signed in to change notification settings - Fork 89
Add CatBoost component and pipeline #247
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #247 +/- ##
==========================================
+ Coverage 97.14% 97.29% +0.15%
==========================================
Files 96 102 +6
Lines 2974 3180 +206
==========================================
+ Hits 2889 3094 +205
- Misses 85 86 +1
Continue to review full report at Codecov.
|
evalml/pipelines/components/estimators/classifiers/catboost_classifier.py
Show resolved
Hide resolved
evalml/pipelines/components/estimators/classifiers/catboost_classifier.py
Show resolved
Hide resolved
evalml/pipelines/components/estimators/classifiers/catboost_classifier.py
Show resolved
Hide resolved
This is great! I left a bunch of little comments, but the only major change request I had was for the |
evalml/pipelines/components/estimators/classifiers/catboost_classifier.py
Show resolved
Hide resolved
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.
Nice work! LGTM 👍
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.
Oops, I thought I approved this yesterday, but I still saw it in the list on slack. Re-approved!
Closes #200
Notes / to discuss:
most_frequent
). Is this too limiting? (Would be nice to apply different impute strategies for different columns...)catboost_info
metadata produced, but good to know. Could we use this when we're considering distributed training? Or at least follow a similar approach?