-
Notifications
You must be signed in to change notification settings - Fork 87
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
Adding some new kwargs to Catboost and LogisticRegression #1202
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1202 +/- ##
==========================================
- Coverage 99.74% 99.74% -0.01%
==========================================
Files 196 196
Lines 12020 11998 -22
==========================================
- Hits 11989 11967 -22
Misses 31 31
Continue to review full report at Codecov.
|
b31da9f
to
b21601b
Compare
b21601b
to
d2a6ff0
Compare
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.
Looks good - really like these changes!
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, this looks clean! I should have chimed in before but allow_writing_files
was previously set to default to False so that evalml wouldn't also dump all of the catboost training logs, but it's also great to provide this as an option to users 😁 Thanks, @freddyaboulton!
f8c3e2a
to
7b5b1e7
Compare
57544c8
to
c505632
Compare
@dsherry Is this good to merge despite codecov/project failure? I think it would be ok because the number of misses has not increased compared to main (still 9). I think the issue is that the coverage in this branch is 11982/11991 = 0.999249, and the coverage in main is 12004/12013 = 0.999250 which is technically a decrease 💀 . Maybe in the future we can change codecov to fail only if the number of missed lines increases? |
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.
@freddyaboulton looks good to me!
@@ -33,16 +32,14 @@ class CatBoostClassifier(Estimator): | |||
SEED_MAX = SEED_BOUNDS.max_bound | |||
|
|||
def __init__(self, n_estimators=10, eta=0.03, max_depth=6, bootstrap_type=None, silent=True, | |||
random_state=0, **kwargs): | |||
allow_writing_files=False, random_state=0, **kwargs): |
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.
Awesome, thanks!
I think we should try to keep it so that if we do anything with the kwargs
params other than passing them along to the component obj, we should declare them as named parameters. It'll keep our code clean.
@freddyaboulton yes let's do it. We probably need to change our codecov thresholds... You want me to merge it? |
@dsherry Yep, I can merge! Thank you |
… to logitistic regression init.
c505632
to
c91b5fd
Compare
Pull Request Description
In #1157, we agreed to add
multi_class
andsolver
kwargs to the logistic regression init (discussion) and addallow_writing_files
to the Catboost init (discussion)This doesn't change the default behavior of these estimators because we are using the same defaults as before.
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.