Repository: scikit-learn/scikit-learn
Pull request: FIX MLPClassifier not iterating to max_iter if warm started
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->

Fixes #16812

#### What does this implement/fix? Explain your changes.

MLPClassifier's `_fit` routine uses
```
 incremental = (self.warm_start and hasattr(self, "classes_"))
```
which, in the case of warm_start and a previous fit call evaluates to
true, thus the fit routine breaks after one iteration instead of
continuing to `max_iter`.
    
The solution requires three changes:
- `MLPClassifier.fit` calls `self._fit` with `incremental = False`
  (similar to MLPRegressor)
- Input validation needs to be updated so that the check for
  compatibility of classes with previous calls is done when `warm_start`
  is true, and either incremental is True or previously created
  results from fit are available in `classes_`
- internal iteration counter needs to be reset to 0 if incremental
  is False, but `warm_start` is True.


#### Any other comments?


<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
warm_start behaviour inconsistent between MLPRegressor and MLPClassifier using SGD
When using `warm_start=True` and `solver="sgd"` in an `MLPClassifier,` if `fit()` has already been called once, subsequent calls to `fit()` only perform one training iteration, regardless of the value `max_iter` is set to. `MLPRegressor` exhibits what I imagine to be the desired behaviour: it trains for another `max_iter` iterations.
This is demonstrated in the following code:

```

from sklearn.datasets import *
from sklearn.neural_network import MLPClassifier, MLPRegressor

                           
print("Regression")
regressor = MLPRegressor(solver='sgd', activation="relu",
                           hidden_layer_sizes=(50, 50),
                           max_iter=3, warm_start=True)
data, labels = load_boston(return_X_y=True)
regressor.fit(data, labels)
print(f"Number of iterations trained initially: {regressor.n_iter_}")
regressor.fit(data, labels)
print(f"Number of iterations trained in total: {regressor.n_iter_}")


print("\n")
print("Classification")
classifier = MLPClassifier(solver='sgd', activation="relu",
                           hidden_layer_sizes=(50, 50),
                           max_iter=3, warm_start=True)
data, labels = load_wine(return_X_y=True)

classifier.fit(data, labels)
print(f"Number of iterations trained initially: {classifier.n_iter_}")
classifier.fit(data, labels)
print(f"Number of iterations trained in total: {classifier.n_iter_}")
```
The output of the above code is:

Regression
Number of iterations trained initially: 3
Number of iterations trained in total: 6

Classification
Number of iterations trained initially: 3
Number of iterations trained in total: 4


Note that the total number of iterations trained in the classification case is 4, rather than 6 as we would expect.

The cause of this is the difference between `fit()` in `MLPClassifier` and `MLPRegressor` (with the latter using the `fit()` method of the `BaseMultiLayerPerceptron.`). `MLPRegressor` has incremental=False, whereas MLPClassifier has incremental=(self.warm_start and `hasattr(self, "classes_")))`. In conjunction with the conditional break at line 565 of `_fit_stochastic()`, this leads to early termination of the training loop in MLPClassifier, and thus causing the difference in behaviour.

Version info:

System:
    python: 3.7.6 (default, Jan  8 2020, 19:59:22)  [GCC 7.3.0]
executable: /home/dwood04/anaconda3/envs/my_env/bin/python
   machine: Linux-4.15.0-91-generic-x86_64-with-debian-buster-sid
Python dependencies:
       pip: 20.0.2
setuptools: 46.0.0.post20200309
   sklearn: 0.22.1
     numpy: 1.18.1
     scipy: 1.4.1
    Cython: None
    pandas: None
matplotlib: 3.1.3
    joblib: 0.14.1
Built with OpenMP: True

---Edit---
This issue should be tagged as a bug, rather than a question.

Hints from issue discussion:
Agreed that warm_start should perform the full set of iterations, unlike
partial_fit.

Commit message: Add a test for Issue 16812: warm starting MLPClassifier

Patch to review:
diff --git a/sklearn/neural_network/_multilayer_perceptron.py b/sklearn/neural_network/_multilayer_perceptron.py
index 368a6b0e5dd56..2e827f6f9f8cf 100644
--- a/sklearn/neural_network/_multilayer_perceptron.py
+++ b/sklearn/neural_network/_multilayer_perceptron.py
@@ -379,6 +379,11 @@ def _fit(self, X, y, incremental=False):
                                            incremental):
             # First time training the model
             self._initialize(y, layer_units, X.dtype)
+        # In case we are warm starting but aren't incremental, the
+        # above routine is not called and the n_iter_ counter is not reset,
+        # do it here.
+        if not incremental:
+            self.n_iter_ = 0
 
         # Initialize lists
         activations = [X] + [None] * (len(layer_units) - 1)
@@ -977,17 +982,18 @@ def _validate_input(self, X, y, incremental):
         if y.ndim == 2 and y.shape[1] == 1:
             y = column_or_1d(y, warn=True)
 
-        if not incremental:
-            self._label_binarizer = LabelBinarizer()
-            self._label_binarizer.fit(y)
-            self.classes_ = self._label_binarizer.classes_
-        elif self.warm_start:
+        if self.warm_start and (incremental or hasattr(self, "classes_")):
             classes = unique_labels(y)
             if set(classes) != set(self.classes_):
                 raise ValueError("warm_start can only be used where `y` has "
                                  "the same classes as in the previous "
                                  "call to fit. Previously got %s, `y` has %s" %
                                  (self.classes_, classes))
+
+        if not incremental:
+            self._label_binarizer = LabelBinarizer()
+            self._label_binarizer.fit(y)
+            self.classes_ = self._label_binarizer.classes_
         else:
             classes = unique_labels(y)
             if len(np.setdiff1d(classes, self.classes_, assume_unique=True)):
@@ -1037,8 +1043,7 @@ def fit(self, X, y):
         -------
         self : returns a trained MLP model.
         """
-        return self._fit(X, y, incremental=(self.warm_start and
-                                            hasattr(self, "classes_")))
+        return self._fit(X, y, incremental=False)
 
     @property
     def partial_fit(self):
diff --git a/sklearn/neural_network/tests/test_mlp.py b/sklearn/neural_network/tests/test_mlp.py
index 9f8cc7d9ed8d1..a551e00b08553 100644
--- a/sklearn/neural_network/tests/test_mlp.py
+++ b/sklearn/neural_network/tests/test_mlp.py
@@ -662,6 +662,17 @@ def test_warm_start():
             clf.fit(X, y_i)
 
 
+def test_warm_start_full_iteration():
+    X = X_iris
+    y = y_iris
+    max_iter = 3
+    clf = MLPClassifier(hidden_layer_sizes=2, solver='sgd',
+                        warm_start=True, max_iter=max_iter)
+    clf.fit(X, y)
+    clf.fit(X, y)
+    assert max_iter == clf.n_iter_
+
+
 def test_n_iter_no_change():
     # test n_iter_no_change using binary data set
     # the classifying fitting process is not prone to loss curve fluctuations


Code review comments:
diff_hunk: @@ -662,6 +662,17 @@ def test_warm_start():
             clf.fit(X, y_i)
 
 
+def test_warm_start_full_iteration():
+    X = X_iris
+    y = y_iris
+    max_iter = 3
+    clf = MLPClassifier(hidden_layer_sizes=2, solver='sgd',
+                        warm_start=True, max_iter=max_iter)
+    clf.fit(X, y)
path: sklearn/neural_network/tests/test_mlp.py
text: @user1:
```suggestion
    clf.fit(X, y)
    assert max_iter == clf.n_iter_
```

@author:
That is a bit hard to read since it is the same code, do you mean something in the vein of
```
clf.fit(X, y)
assert max_iter == clf.n_iter_
clf.fit(X, y)
assert max_iter == clf.n_iter_
```
That is, testing two times that `max_iter` has been reached?

@author:
Done that with the last set of commits pushed.

diff_hunk: @@ -1037,8 +1043,7 @@ def fit(self, X, y):
         -------
         self : returns a trained MLP model.
         """
-        return self._fit(X, y, incremental=(self.warm_start and
-                                            hasattr(self, "classes_")))
+        return self._fit(X, y, incremental=False)
path: sklearn/neural_network/_multilayer_perceptron.py
text: @user1:
We can actually remove the `fit` function because it is the same as the base class then.

@author:
Indeed indeed, good point! I will update the PR.

Do you prefer a push --force with the initial commit removing the function, or a commit on top of the current status?

@user2:
We prefer to commit on top because it helps us keep the context of the comments on github.

@author:
Thanks @user2
I'll send new commits after the weekend (currently sweating the hell out of it climbing).

@author:
Done that with the last set of commits pushed.

diff_hunk: @@ -662,6 +662,17 @@ def test_warm_start():
             clf.fit(X, y_i)
 
 
+def test_warm_start_full_iteration():
path: sklearn/neural_network/tests/test_mlp.py
text: @user2:
We should parametrize the test with pytest and check that we have the same behaviour with the regressor and classifier

```suggestion
@user1.mark.parametrize("MLPEstimator", [MLPClassifier, MLPRegressor])
def test_warm_start_full_iteration(MLPEstimator):
    # Non-regression test for:
    # https://github.com/scikit-learn/scikit-learn/issues/16812
    # Check that the MLP estimator only accomplish `max_iter` with a
    # warm started estimator.
```

@author:
Good point, didn't think of that. I will look into it and update the PR (but leaving for a weekend of mountaineering soon)

@author:
Done that with the last set of commits pushed