search

dask / dask-xgboost

BSD 3-Clause "New" or "Revised" License
162 stars 43 forks source link

Early stopping support classifier and regressor fit #54

Closed mmccarty closed 5 years ago

mmccarty commented 5 years ago

This change adds support for early stopping to the sklearn interface by passing arguments in the proper format.

fixes dask/dask-xgboost#38

mmccarty commented 5 years ago

It would be better to refactor certain parts of sklearn API code in xgboost to be more reusable here rather than duplicating the code. Once this settles out, I would like to clean that up.

mmccarty commented 5 years ago

@TomAugspurger Thanks for taking a look. If folks are good with this approach, I will proceed.

TomAugspurger commented 5 years ago

So to summarize the discussion from https://github.com/dask/dask-xgboost/pull/54/files/d10bb355a1d82a89233c58295c2b515d3e39baed#diff-66bc4b86e5e634f64f45b16394051674, we decided that eval_set and sample_weight_eval_set should be in-memory ndarrays, which are sent to each worker? That matches what we do with Hyperband in dask-ml.

Eventually we'll want to support sample_weight, which IIUC will be a dask array the same length as X and y.

TomAugspurger commented 5 years ago

Overall, things are looking good here @mmccarty. I think just linting issues now.

mmccarty commented 5 years ago

Great! Thank you for checking. I’ll get the linting fixed ASAP.

On Tue, Oct 1, 2019 at 6:23 PM Tom Augspurger notifications@github.com 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 https://github.com/dask/dask-xgboost/pull/54?email_source=notifications&email_token=AAEY2GUZW347YUTSLIS7OITQMPEUJA5CNFSM4I2EVIZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAC6L3I#issuecomment-537257453, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEY2GQZI4QMDMT4SRZYFF3QMPEUJANCNFSM4I2EVIZA .

mmccarty commented 5 years ago

@TomAugspurger CI is passing. I also want to add this to the Regressor. I'm working on that now.

mmccarty commented 5 years ago

@TomAugspurger @mrocklin All done here, unless there are further comments.

TomAugspurger commented 5 years ago

Alrightly, let's merge this. We can followup if xgboost decides to change their behavior.

Thanks @mmccarty!

TomAugspurger commented 5 years ago

Is there a time where a release would be good for you? I can do one sometime next week if that works.

mmccarty commented 5 years ago

Great! Thanks! Yeah, sometime next week works!