FEA Turn on early stopping in histogram GBDT by default #14516

johannfaouzi · 2019-07-30T07:48:46Z

Reference Issues/PRs

Fixes #14503.

What does this implement/fix? Explain your changes.

This PR enables early stopping by default in HGBM:

n_iter_no_change=10 instead of n_iter_no_change=None (not sure about this value, I looked at Should we turn on early stopping in HistGradientBoosting by default? #14303)
The docstrings have been changed accordingly
The following sentence has been added in the docstrings (changes are welcomed, lack of inspiration): Early stopping is the default behavior, as it usually makes the fitting process much faster without a substantial difference in terms of predictive performance.
The random_state has to be fixed in the examples (for the cross-validation), because the performance (clf.score(X, y)) depends on the splitting.
n_iter_no_change=None has been added to some existing tests, so that they can check the expected behavior.
A small test has been added that checks that early stopping is enabled by default

johannfaouzi · 2019-07-30T08:16:21Z

It looks like HistGradientBoostingClassifier does not pass test_estimators...

Most common errors:

ValueError: The test_size = 1 should be greater or equal to the number of classes = 3
ValueError: The test_size = 2 should be greater or equal to the number of classes = 3

This error also occurs one time in test_estimators[HistGradientBoostingClassifier-check_classifiers_classes]:

TypeError: '<' not supported between instances of 'str' and 'float'

NicolasHug

Thanks for giving this a shot @johannfaouzi.

Please add a simple parametrized test that makes sure ES is enabled by default.

Regarding the estimator checks failures, I think the simplest way to go for now is to update set_checking_parameters in utils/estimator_checks.py and deactivate early stopping here.

sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py

johannfaouzi · 2019-07-31T05:50:07Z

Thanks for the help regarding test_estimators! Is test_early_stopping_default not enough? I added this small test in test_gradient_boosting.py

…

Le 30 juil. 2019 à 20:42, Nicolas Hug ***@***.***> a écrit : @NicolasHug commented on this pull request. Thanks for giving this a shot @johannfaouzi. Please add a simple parametrized test that makes sure ES is enabled by default. Regarding the estimator checks failures, I think the simplest way to go for now is to set_checking_parameters in utils/estimator_checks.py and deactivate early stopping here. In sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py: > @@ -622,7 +622,9 @@ class HistGradientBoostingRegressor(BaseHistGradientBoosting, RegressorMixin): for big datasets (n_samples >= 10 000). The input data ``X`` is pre-binned into integer-valued bins, which considerably reduces the number of splitting points to consider, and allows the algorithm to leverage - integer-based data structures. For small sample sizes, + integer-based data structures. Early stopping is the default behavior, as I'd rather have this after or at the end of this paragraph — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

NicolasHug

Nitpic but LGTM.
Thanks @johannfaouzi !!

So the new behavior is that early stopping is enabled with n_iter_no_change=10, validation_fraction is .1 and the scorer is the estimator's default scorer. I'll make sure to document this in the user guide in #14525 .

sklearn/ensemble/_hist_gradient_boosting/tests/test_gradient_boosting.py

NicolasHug · 2019-07-31T13:01:27Z

@johannfaouzi , we're planning to add support for sample weights. It's a non trivial change since that would require changes pretty much everywhere in the GBDT code, including (and mostly) in Cython.

We'd be happy to provide guidance if that's something you'd like to work on? I'd definitely tag this as a hard PR, but it could be a great learning experience ;). LMK.

NicolasHug · 2019-07-31T14:26:16Z

@adrinjalali might want to review this?

johannfaouzi · 2019-07-31T14:45:10Z

@johannfaouzi , we're planning to add support for sample weights. It's a non trivial change since that would require changes pretty much everywhere in the GBDT code, including (and mostly) in Cython.

We'd be happy to provide guidance if that's something you'd like to work on? I'd definitely tag this as a hard PR, but it could be a great learning experience ;). LMK.

Thanks for your proposal! Unfortunately I think that it would be a bit too time-consuming for me, as I have never written a single line of Cython (I am a big fan of numba thus). I saw that you are working on a pure-numba implementation of GBM in pygbm with Olivier. Can't wait for numba to be added as a dependency of scikit-learn, although it might be a while before it happens ;)

NicolasHug · 2019-07-31T14:54:30Z

No problem!

Can't wait for numba to be added as a dependency of scikit-learn, although it might be a while before it happens ;)

Ha, I don't see that happening anytime soon. When we were developing pygbm I found numba to be fairly unstable compared to Cython. And as far as I know they still don't support compilation caching which is mandatory for us (pygbm takes about 10 seconds to compile every single time you run it). And I'm not even talking about the fact that we don't like adding dependencies ^^

johannfaouzi · 2019-07-31T15:01:51Z

And as far as I know they still don't support compilation caching which is mandatory for us (pygbm takes about 10 seconds to compile every single time you run it).

There has been a cache option for a while, so it's probably not what you mean (but I clearly lack knowledge in that part of IT to clearly understand the difference).

And I'm not even talking about the fact that we don't like adding dependencies ^^

That's what I thought :)

amueller · 2019-07-31T16:25:54Z

@NicolasHug scipy is going to add numba as a dependency soon(ish), so sklearn will have it by default... at least if Ralf get's his way ;)

adrinjalali · 2019-07-31T18:01:47Z

sklearn/inspection/tests/test_partial_dependence.py

-    (HistGradientBoostingRegressor(random_state=0), 'recursion')]
+    (HistGradientBoostingRegressor(random_state=0, n_iter_no_change=None),
+     'brute'),
+    (HistGradientBoostingRegressor(random_state=0, n_iter_no_change=None),


why would the test fail with early stopping?

there are very small training sets where early stopping leads to bad results

the dataset has 100 samples (the default value for our make_regression), right? I kinda feel like the default parameters should give reasonable results for the default parameters of our data generators. Would that be too hard to have an auto early stopping parameter to figure if it's a good idea or not?

I really miss having a nice and more dynamic early stopping parameter, like a policy, a function, or an object.

I think I remember a dataset with 40 samples. Not sure.

Would that be too hard to have an auto early stopping parameter to figure if it's a good idea or not?

I don't think that's the place for this discussion.

See also the latest comments on #11324

The tests fails with early stopping, otherwise I would not have changed this (I didn't know about the inspection module before). I can undo the changes locally to see where it fails exactly if you are interested.

@NicolasHug I think this is a relevant place to discuss this here, because clearly we're changing our defaults in a way that makes the defaults break on a lot of our examples. That kind of brings into question whether that's actually an improvement. Having to change the doctests to adhere to the lower accuracies is not a good sign either.

In other words, I think this has less to do with how to test, and more with what good default settings are.

Ok, I meant to say that if we want to have a discussion around reworking our early-stopping mechanism, we should probably open an issue for it.

I'm not particularly uncomfortable with estimators not passing our common tests with the default values. I don't even think we can achieve that.

Take the first failing test, check_estimators_dtypes. This one has a dataset with n_samples=20 and 3 classes. The default for validation_fraction is .1 which leads to test_size=2, so train_test_split fails with The test_size = 2 should be greater or equal to the number of classes = 3

What is the right course of action here? I don't think it's worth changing the default, or having a "clever" 'auto' value just for this test to pass. It's not worth changing the test either. This would be another form of the VW thing that you mentionned before. The test isn't even remotely related to accuracy/overfitting (so it's not related to early stopping either: there's no harm in disabling it)

adrinjalali · 2019-07-31T18:06:38Z

Otherwise LGTM

johannfaouzi · 2019-08-03T10:43:59Z

As mentioned in the docstrings, HistGBM is not really suited for small datasets. However, the tests use relatively small datasets (it makes them faster). Moreover, on small datasets with early stopping, the validation set is very small and makes the variance of the performance evaluation quite high.

Maybe we should add a new parameter early_stopping:

early_stopping : 'auto' or bool (default='bool')
    If 'auto', early stopping is enabled if the sample size is larger than 1000.
    If True, early stopping is enabled. If False, early stopping is disabled.

1000 is just a placeholder, I don't know what a good value could be.

n_iter_no_change would always be an integer and would be ignored if there is no early stopping.

adrinjalali · 2019-08-05T12:14:50Z

I would agree with an auto option, and having it the default.

NicolasHug · 2019-08-05T12:21:22Z

I'm OK with an 'auto' default but I personally have no idea what a sensible default would be. Maybe @ogrisel would have some ideas.

…into early_stopping_HGBM

NicolasHug

Thanks @johannfaouzi , still LGTM.

The test_warm_start_early_stopping should be updated to:

set early_stopping=True
change the assert to assert 0 < n_iter_second_fit - n_iter_first_fit < n_iter_no_change to avoid the test succeeding for the wrong reason (like it is right now)

doc/modules/ensemble.rst

NicolasHug · 2020-01-31T16:25:22Z

Could this get some love maybe @ogrisel @adrinjalali @glemaitre ?

…-14516

…learn into pr-14516

adrinjalali

This PR somehow exploded, and it was partly my fault 🙈

I like the changes now. Do we test for effectiveness of it somehow? i.e. do we check if it improves the performance?

adrinjalali · 2020-02-03T13:45:56Z

doc/whats_new/v0.23.rst

+- |Feature| :func:`inspection.partial_dependence` and
+  :func:`inspection.plot_partial_dependence` now support the fast 'recursion'
+  method for both estimators. :pr:`13769` by `Nicolas Hug`_.
+
 :mod:`sklearn.feature_extraction`


adrinjalali · 2020-02-03T13:47:03Z

sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py

@@ -710,21 +712,25 @@ class HistGradientBoostingRegressor(RegressorMixin, BaseHistGradientBoosting):
        and add more estimators to the ensemble. For results to be valid, the
        estimator should be re-trained on the same data only.
        See :term:`the Glossary <warm_start>`.
-    scoring : str or callable or None, optional (default=None)
+    early_stopping : 'auto' or bool (default='auto')


Suggested change

early_stopping : 'auto' or bool (default='auto')

early_stopping : 'auto' or bool, default='auto'

but also happy to have all of them fixed in another PR

yeah we should probably have the whole file fixed in another PR since all docstrings follow the old style rn

NicolasHug · 2020-02-03T14:10:15Z

Do we test for effectiveness of it somehow? i.e. do we check if it improves the performance?

Benchmarks start here #14516 (comment)

adrinjalali

ah yeah now I remember, at the end there wasn't something which we could add to the tests easily.

NicolasHug · 2020-02-03T15:23:30Z

Thanks @adrinjalali

Codecov seems unhappy but it's giving me a 500

@ogrisel I'll merge in the following days unless you have any additional comments

glemaitre

LGTM. I think that we could move forward @NicolasHug

NicolasHug · 2020-02-12T15:44:58Z

Yup thanks for the reminder!

NicolasHug · 2020-02-12T15:45:35Z

Thanks for the nice work @johannfaouzi !

glemaitre · 2020-02-12T15:47:31Z

Thanks @johannfaouzi

…#14516)

johann.faouzi added 4 commits July 30, 2019 09:05

Update docstrings and change default value

b1bc2de

Disable early stopping for some tests

c30e4d9

Check that early stopping is enabled by default

a196c5e

Fix the random state in the examples

b84c7e2

johannfaouzi changed the title ~~[MRG]: Turn on early stopping in HGBM by default~~ [WiP] Turn on early stopping in HGBM by default Jul 30, 2019

NicolasHug reviewed Jul 30, 2019

View reviewed changes

sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py Outdated Show resolved Hide resolved

NicolasHug mentioned this pull request Jul 30, 2019

[MRG] DOC User guide section for histogram-based GBDTs #14525

Merged

johann.faouzi added 3 commits July 31, 2019 10:01

Move sentence at the end of the paragraph

c02faea

Disable early stopping in test_estimators

de41ab3

Disable early stopping in partial dependence tests

9e83463

NicolasHug previously approved these changes Jul 31, 2019

View reviewed changes

sklearn/ensemble/_hist_gradient_boosting/tests/test_gradient_boosting.py Outdated Show resolved Hide resolved

NicolasHug changed the title ~~[WiP] Turn on early stopping in HGBM by default~~ [MRG] Turn on early stopping in histogram GBDT by default Jul 31, 2019

Move the new test next to the others tests regarding ES

6f332d0

adrinjalali reviewed Jul 31, 2019

View reviewed changes

johann.faouzi added 4 commits August 6, 2019 15:27

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

460a307

…into early_stopping_HGBM

Update the docstrings and the init for both classes

5764fac

Swap warm_start and early_stopping

42f3d95

Update validation_fraction documentation

16a83e8

johannfaouzi added 4 commits November 15, 2019 15:04

Remove private attributes for raw predictions

7f81df7

Revert changes

58c9bce

Revert changes

4e093ad

Merge branch 'master' into early_stopping_HGBM

52ca216

NicolasHug approved these changes Nov 15, 2019

View reviewed changes

doc/modules/ensemble.rst Outdated Show resolved Hide resolved

johannfaouzi added 2 commits November 15, 2019 18:16

Add note about scorer

6ed0735

Fix test_warm_start_early_stopping

29a3722

qinhanmin2014 mentioned this pull request Nov 27, 2019

Release 0.22rc3 #15715

Merged

adrinjalali modified the milestones: 0.22, 0.23 Dec 3, 2019

ogrisel self-requested a review December 23, 2019 13:16

NicolasHug mentioned this pull request Jan 31, 2020

documentation of attributes of HistGradientBoostingClassifier except … #16283

Closed

NicolasHug added 2 commits January 31, 2020 11:29

Merge branch 'master' of github.com:scikit-learn/scikit-learn into pr…

8827fb5

…-14516

Merge branch 'early_stopping_HGBM' of github.com:johannfaouzi/scikit-…

5f65675

…learn into pr-14516

adrinjalali reviewed Feb 3, 2020

View reviewed changes

fixed bad merge

82a3796

adrinjalali approved these changes Feb 3, 2020

View reviewed changes

Fixed LightGBM tests: properly deactive ES since parameters have changed

6f2b70a

glemaitre approved these changes Feb 12, 2020

View reviewed changes

NicolasHug changed the title ~~[MRG] Turn on early stopping in histogram GBDT by default~~ FEA Turn on early stopping in histogram GBDT by default Feb 12, 2020

NicolasHug merged commit ee6b369 into scikit-learn:master Feb 12, 2020

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020

FEA Turn on early stopping in histogram GBDT by default (scikit-learn…

54c7fe2

…#14516)

panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020

FEA Turn on early stopping in histogram GBDT by default (scikit-learn…

9e3214c

…#14516)

thomasjpfan mentioned this pull request Mar 10, 2020

FIX Fixes HistGradientBoosting bug fail when early stopping + no validation + warm starting #16662

Closed

	early_stopping : 'auto' or bool (default='auto')
	early_stopping : 'auto' or bool, default='auto'

FEA Turn on early stopping in histogram GBDT by default #14516

FEA Turn on early stopping in histogram GBDT by default #14516

Conversation

johannfaouzi commented Jul 30, 2019

Reference Issues/PRs

What does this implement/fix? Explain your changes.

johannfaouzi commented Jul 30, 2019 • edited Loading

NicolasHug left a comment • edited Loading

Choose a reason for hiding this comment

johannfaouzi commented Jul 31, 2019 via email

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug commented Jul 31, 2019

NicolasHug commented Jul 31, 2019

johannfaouzi commented Jul 31, 2019

NicolasHug commented Jul 31, 2019

johannfaouzi commented Jul 31, 2019

amueller commented Jul 31, 2019

Choose a reason for hiding this comment

NicolasHug Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali commented Jul 31, 2019

johannfaouzi commented Aug 3, 2019

adrinjalali commented Aug 5, 2019

NicolasHug commented Aug 5, 2019

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug commented Jan 31, 2020

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug commented Feb 3, 2020

adrinjalali left a comment

Choose a reason for hiding this comment

NicolasHug commented Feb 3, 2020

glemaitre left a comment

Choose a reason for hiding this comment

NicolasHug commented Feb 12, 2020

NicolasHug commented Feb 12, 2020

glemaitre commented Feb 12, 2020 • edited Loading

johannfaouzi commented Jul 30, 2019 •

edited

Loading

NicolasHug left a comment •

edited

Loading

NicolasHug Jul 31, 2019 •

edited

Loading

glemaitre commented Feb 12, 2020 •

edited

Loading