-
-
Notifications
You must be signed in to change notification settings - Fork 42
Early stopping support classifier and regressor fit #54
Conversation
|
It would be better to refactor certain parts of |
|
@TomAugspurger Thanks for taking a look. If folks are good with this approach, I will proceed. |
|
So to summarize the discussion from https://github.com/dask/dask-xgboost/pull/54/files/d10bb355a1d82a89233c58295c2b515d3e39baed#diff-66bc4b86e5e634f64f45b16394051674, we decided that Eventually we'll want to support |
|
Overall, things are looking good here @mmccarty. I think just linting issues now. |
|
Great! Thank you for checking. I’ll get the linting fixed ASAP.
…On Tue, Oct 1, 2019 at 6:23 PM Tom Augspurger ***@***.***> wrote:
Overall, things are looking good here @mmccarty
<https://github.com/mmccarty>. I think just linting issues now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#54?email_source=notifications&email_token=AAEY2GUZW347YUTSLIS7OITQMPEUJA5CNFSM4I2EVIZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAC6L3I#issuecomment-537257453>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEY2GQZI4QMDMT4SRZYFF3QMPEUJANCNFSM4I2EVIZA>
.
|
|
@TomAugspurger CI is passing. I also want to add this to the Regressor. I'm working on that now. |
b48d394 to
df3cf12
Compare
|
@TomAugspurger @mrocklin All done here, unless there are further comments. |
| weight=weight, | ||
| nthread=n_jobs, | ||
| ) | ||
| for ((data, label), weight) in zip( |
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.
Can you check: what happens if the user does eval_set=[(x1, y2), (x2, y2)], sample_weight_eval_set=[weight1]? Do we sielently drop the (x2, y2) eval set here, since the lengths don't match?
Would the equivalent mistake raise in xgboost, or do they silently proceed?
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.
I'll check.
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.
@TomAugspurger I copied this code from xgboost here and then cleaned it up a bit. The original code will silently proceed. This code will error. I'm going to align it with the original code.
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.
Actually, no it doesn't error. They behave the same. I'll push up another unit test to confirm.
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.
Heh, OK.
FWIW, I think raising is the right thing to do... Maybe open an issue with XGBoost to see if they agree? Then you won't need to change anything here.
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.
That's what I was wondering. What's the right thing to do? Three options, maybe?
- Silently drop
- Silently fill right
- Raise error
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.
@TomAugspurger I opened an issue. Good catch!
|
Alrightly, let's merge this. We can followup if xgboost decides to change their behavior. Thanks @mmccarty! |
|
Is there a time where a release would be good for you? I can do one sometime next week if that works. |
|
Great! Thanks! Yeah, sometime next week works! |
This change adds support for early stopping to the sklearn interface by passing arguments in the proper format.
fixes #38