Repository: scikit-learn/scikit-learn
Pull request: [MRG] Using one single random seed for warm start in HGBDT
#### Reference Issues/PRs

Discussion from #14034. **This PR is for the histogram-based version of GBDT.**

#### What does this implement/fix? Explain your changes.

One single random seed is used instead of several ones. It is needed to save it for warm starting, so that every time there is some sort of RNG, the same seed is used.

* `self._small_trainset_seed = rng.randint(1024)` and `self._train_val_split_seed = rng.randint(1024)` have been replaced with `self._random_seed = rng.randint(np.iinfo(np.uint32).max)` ([@ogrisel  comment](https://github.com/scikit-learn/scikit-learn/issues/14034#issuecomment-531150174))

* The random seed is passed to the bin mapper ([@NicolasHug comment](https://github.com/scikit-learn/scikit-learn/issues/14034#issuecomment-531563182)). The `rng` parameter in the [_bin_data](https://github.com/scikit-learn/scikit-learn/blob/d2476fb679f05e80c56e8b151ff0f6d7a470e4ae/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py#L491-L513) is removed since it is not used, and we pass the `rng` parameter to `BinMapper` [two lines above](https://github.com/johannfaouzi/scikit-learn/blob/9b83c211a46a05821ffb825f255a9a525b074e89/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py#L158-L159).

#### Any other comments?

When we have a version of warm starting that is good enough for HGBDT, I will try to do the same for GBDT (i.e. fix #14034).

Possible reviewers: @NicolasHug @ogrisel 

Problem statement:
data leak in GBDT due to warm start
(This is about the non-histogram-based version of GBDTs)

X is split into train and validation data with `train_test_split(random_state=self.random_state)`.

As @johannfaouzi noted, in a warm starting context, this will produce a leak if If `self.random_state` is a `RandomState` instance: some samples that were used for training in a previous `fit` might be used for validation now.

~~I think the right fix would be to raise a `ValueError` if the provided random state isn't a number and early-stopping is activated~~

Hints from issue discussion:
After discussing with @amueller 	, maybe the best option would be to:

- store a seed attribute e.g. `_train_val_split_seed` that would be generated **once**, the first time `fit` is called
- pass this seed as the `random_state`  parameter to `train_test_split()`.
- add a small test making sure this parameter stays constant between different calls to `fit`

This should only be done when warm_start is true, though. So different calls to ``fit`` without warm start should probably use different splits if random state was None or a random state object.
Right.

The problem is that we allow warm_start to be False the first time we fit, and later set it to true with `set_params` :(
Sorry, we should always store the seed, but only re-use it if ``warm_start=True``.
Why do you prefer to store a seed over `get_state()`
At the end of the day, something has to be stored in order to generate the same training and validation sets. An integer is smaller than a tuple returned by `get_state()`, but the difference can probably be overlooked.
If we store a seed we can just directly pass it to `train_test_split`, avoiding the need to create a new RandomState instance.

both are fine with me. 
+1 for storing a seed as fit param and reuse that to seed an rng in fit only when `warm_start=True`.

AFAIK, `np.random.RandomState` accept `uint32` seed only (between `0` and `2**32 - 1`). So the correct way to get a seed from an existing random state object is:

```python
self.random_seed_ = check_random_state(self.random_state).randint(np.iinfo(np.uint32).max)
```
I did something that is not as good for HGBDT:
https://github.com/scikit-learn/scikit-learn/blob/06632c0d185128a53c57ccc73b25b6408e90bb89/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py#L142
It would probably be worth changing this as well.

I can work on this if there is no one.
PR welcome @johannfaouzi :)

Also instead of having `_small_trainset_seed` and `_train_val_seed` maybe we can just have one single seed. And I just realized that seed should also be passed to the binmapper that will also subsample.

Commit message: Fix dtype when generating random seed for 32 bit versions

Patch to review:
diff --git a/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py b/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py
index b5c2d2b77f841..269f1086d193c 100644
--- a/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py
+++ b/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py
@@ -105,11 +105,10 @@ def fit(self, X, y):
         y = self._encode_y(y)
 
         # The rng state must be preserved if warm_start is True
-        if (self.warm_start and hasattr(self, '_rng')):
-            rng = self._rng
-        else:
-            rng = check_random_state(self.random_state)
-            self._rng = rng
+        rng = check_random_state(self.random_state)
+        if not (self.warm_start and self._is_fitted()):
+            self._random_seed = rng.randint(np.iinfo(np.uint32).max,
+                                            dtype='u8')
 
         self._validate_parameters()
         self.n_features_ = X.shape[1]  # used for validation in predict()
@@ -138,12 +137,10 @@ def fit(self, X, y):
             # Save the state of the RNG for the training and validation split.
             # This is needed in order to have the same split when using
             # warm starting.
-            if not (self._is_fitted() and self.warm_start):
-                self._train_val_split_seed = rng.randint(1024)
 
             X_train, X_val, y_train, y_val = train_test_split(
                 X, y, test_size=self.validation_fraction, stratify=stratify,
-                random_state=self._train_val_split_seed)
+                random_state=self._random_seed)
         else:
             X_train, y_train = X, y
             X_val, y_val = None, None
@@ -159,10 +156,11 @@ def fit(self, X, y):
         # actual total number of bins. Everywhere in the code, the
         # convention is that n_bins == max_bins + 1
         n_bins = self.max_bins + 1  # + 1 for missing values
-        self.bin_mapper_ = _BinMapper(n_bins=n_bins, random_state=rng)
-        X_binned_train = self._bin_data(X_train, rng, is_training_data=True)
+        self.bin_mapper_ = _BinMapper(n_bins=n_bins,
+                                      random_state=self._random_seed)
+        X_binned_train = self._bin_data(X_train, is_training_data=True)
         if X_val is not None:
-            X_binned_val = self._bin_data(X_val, rng, is_training_data=False)
+            X_binned_val = self._bin_data(X_val, is_training_data=False)
         else:
             X_binned_val = None
 
@@ -241,13 +239,10 @@ def fit(self, X, y):
                     # the predictions of all the trees. So we use a subset of
                     # the training set to compute train scores.
 
-                    # Save the seed for the small trainset generator
-                    self._small_trainset_seed = rng.randint(1024)
-
                     # Compute the subsample set
                     (X_binned_small_train,
                      y_small_train) = self._get_small_trainset(
-                        X_binned_train, y_train, self._small_trainset_seed)
+                        X_binned_train, y_train, self._random_seed)
 
                     self._check_early_stopping_scorer(
                         X_binned_small_train, y_small_train,
@@ -276,7 +271,7 @@ def fit(self, X, y):
             if self.do_early_stopping_ and self.scoring != 'loss':
                 # Compute the subsample set
                 X_binned_small_train, y_small_train = self._get_small_trainset(
-                    X_binned_train, y_train, self._small_trainset_seed)
+                    X_binned_train, y_train, self._random_seed)
 
             # Initialize the gradients and hessians
             gradients, hessians = self.loss_.init_gradients_and_hessians(
@@ -400,7 +395,7 @@ def _is_fitted(self):
 
     def _clear_state(self):
         """Clear the state of the gradient boosting model."""
-        for var in ('train_score_', 'validation_score_', '_rng'):
+        for var in ('train_score_', 'validation_score_'):
             if hasattr(self, var):
                 delattr(self, var)
 
@@ -488,7 +483,7 @@ def _should_stop(self, scores):
                                for score in recent_scores]
         return not any(recent_improvements)
 
-    def _bin_data(self, X, rng, is_training_data):
+    def _bin_data(self, X, is_training_data):
         """Bin data X.
 
         If is_training_data, then set the bin_mapper_ attribute.
diff --git a/sklearn/ensemble/_hist_gradient_boosting/tests/test_warm_start.py b/sklearn/ensemble/_hist_gradient_boosting/tests/test_warm_start.py
index 806ad94ccee98..ccb616393c2cc 100644
--- a/sklearn/ensemble/_hist_gradient_boosting/tests/test_warm_start.py
+++ b/sklearn/ensemble/_hist_gradient_boosting/tests/test_warm_start.py
@@ -169,22 +169,16 @@ def _get_rng(rng_type):
     gb_1 = GradientBoosting(n_iter_no_change=5, max_iter=2,
                             random_state=random_state)
     gb_1.fit(X, y)
-    train_val_seed_1 = gb_1._train_val_split_seed
-    small_trainset_seed_1 = gb_1._small_trainset_seed
+    random_seed_1 = gb_1._random_seed
 
     random_state = _get_rng(rng_type)
     gb_2 = GradientBoosting(n_iter_no_change=5, max_iter=2,
                             random_state=random_state, warm_start=True)
     gb_2.fit(X, y)  # inits state
-    train_val_seed_2 = gb_2._train_val_split_seed
-    small_trainset_seed_2 = gb_2._small_trainset_seed
+    random_seed_2 = gb_2._random_seed
     gb_2.fit(X, y)  # clears old state and equals est
-    train_val_seed_3 = gb_2._train_val_split_seed
-    small_trainset_seed_3 = gb_2._small_trainset_seed
+    random_seed_3 = gb_2._random_seed
 
     # Check that all seeds are equal
-    assert train_val_seed_1 == train_val_seed_2
-    assert small_trainset_seed_1 == small_trainset_seed_2
-
-    assert train_val_seed_2 == train_val_seed_3
-    assert small_trainset_seed_2 == small_trainset_seed_3
+    assert random_seed_1 == random_seed_2
+    assert random_seed_2 == random_seed_3


Code review comments:
diff_hunk: @@ -169,22 +169,16 @@ def _get_rng(rng_type):
     gb_1 = GradientBoosting(n_iter_no_change=5, max_iter=2,
                             random_state=random_state)
     gb_1.fit(X, y)
-    train_val_seed_1 = gb_1._train_val_split_seed
-    small_trainset_seed_1 = gb_1._small_trainset_seed
+    random_seed_1 = gb_1._random_seed
path: sklearn/ensemble/_hist_gradient_boosting/tests/test_warm_start.py
text: @user1:
for completeness let's refit `gb_1` and assert that the seed is different here

@author:
~~There is still a small chance (2 ** -32) that the seeds are equal. Let's hope that it never happens :)~~

The seeds will be equal when the random state is an integer. They will only be different (with high probability) when the random seed is a RandomState instance (because it will be mutated) or None.

@user1:
Actually never mind I think it's expected that the seed will be equal anyway since we pass an int and a fixed instance as `random_state`. I guess we should pass `None` instead?

diff_hunk: @@ -105,11 +105,10 @@ def fit(self, X, y):
         y = self._encode_y(y)
 
         # The rng state must be preserved if warm_start is True
path: sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py
text: @user1:
that comment is outdated. Maybe replace with e.g.:

When warm starting, we want to re-use the same seed that was used the first time fit was called (e.g. for subsampling or for the train/val split).

diff_hunk: @@ -169,22 +169,16 @@ def _get_rng(rng_type):
     gb_1 = GradientBoosting(n_iter_no_change=5, max_iter=2,
                             random_state=random_state)
     gb_1.fit(X, y)
-    train_val_seed_1 = gb_1._train_val_split_seed
-    small_trainset_seed_1 = gb_1._small_trainset_seed
+    random_seed_1 = gb_1._random_seed
 
     random_state = _get_rng(rng_type)
     gb_2 = GradientBoosting(n_iter_no_change=5, max_iter=2,
                             random_state=random_state, warm_start=True)
     gb_2.fit(X, y)  # inits state
-    train_val_seed_2 = gb_2._train_val_split_seed
-    small_trainset_seed_2 = gb_2._small_trainset_seed
+    random_seed_2 = gb_2._random_seed
     gb_2.fit(X, y)  # clears old state and equals est
-    train_val_seed_3 = gb_2._train_val_split_seed
-    small_trainset_seed_3 = gb_2._small_trainset_seed
+    random_seed_3 = gb_2._random_seed
 
     # Check that all seeds are equal
-    assert train_val_seed_1 == train_val_seed_2
-    assert small_trainset_seed_1 == small_trainset_seed_2
-
-    assert train_val_seed_2 == train_val_seed_3
-    assert small_trainset_seed_2 == small_trainset_seed_3
+    assert random_seed_1 == random_seed_2
path: sklearn/ensemble/_hist_gradient_boosting/tests/test_warm_start.py
text: @user1:
as a nit `a == b == c` works as expected in Python