Repository: scikit-learn/scikit-learn
Pull request: FIX report properly `n_iter_` when `warm_start=True`
closes #24764
closes #8713
related to https://github.com/scikit-learn/scikit-learn/issues/25522

In `MLPEstimator`, `n_iter_` does not report the number of iterations of the `fit` call but rather the total number of iterations since the initialization. This is inconsistent with the other estimators as reported in #25522.

This PR solves the issue.

Problem statement:
With MLPClassifer, when warm_start is True or coeffs_ are provided, fit doesn’t respect max_iters
#### Description
With MLPClassifer, when warm_start is True or coeffs_ are provided, fit doesn’t respect max_iters. The reason for this is, when fitting, max iteration check is equality (==) against self.n_iter_. When warm_start is true or coeffs_ are provided, initialize is not called; this method resets n_iter_ to 0. Based on this implementation, there is doubt as to the meaning of max_iter. Consider, if max_iter is 1 and fit terminates due to reaching maximum iterations, subsequent fittings with warm_start true will never terminate due to reaching maximum iterations. This is bug. An alternate interpretation is max_iter represents the maximum iterations per fit call. In this case, the implementation is also wrong. The later interpretation seems more reasonable.

#### Steps/Code to Reproduce
```
import numpy as np
from sklearn.neural_network import MLPClassifier

X = np.random.rand(100,10)
y = np.random.random_integers(0, 1, (100,))

clf = MLPClassifier(max_iter=1, warm_start=True, verbose=True)
for k in range(3):
    clf.fit(X, y)
```
#### Expected Results
Iteration 1, loss = 0.72311215
ConvergenceWarning: Stochastic Optimizer: Maximum iterations reached and the optimization hasn't converged yet.
Iteration 2, loss = 0.71843526
ConvergenceWarning: Stochastic Optimizer: Maximum iterations reached and the optimization hasn't converged yet.
Iteration 3, loss = 0.71418678
ConvergenceWarning: Stochastic Optimizer: Maximum iterations reached and the optimization hasn't converged yet.

#### Actual Results
Iteration 1, loss = 0.72311215
ConvergenceWarning: Stochastic Optimizer: Maximum iterations reached and the optimization hasn't converged yet.
Iteration 2, loss = 0.71843526
Iteration 3, loss = 0.71418678

#### Versions
Windows-7-6.1.7601-SP1
Python 3.6.0 (v3.6.0:41df79263a11, Dec 23 2016, 08:06:12) [MSC v.1900 64 bit (AMD64)]
NumPy 1.12.0
SciPy 0.18.1
Scikit-Learn 0.18.1



MLPEstimator do not report the proper `n_iter_` after successive call of `fit`
### Describe the bug

It seems that `MLPRegressor` and `MLPClassifier` does not take into account `max_iter` with `warm_start`.

### Steps/Code to Reproduce


```python
model = MLPRegressor(warm_start=True, early_stopping=True, max_iter=10)
model.fit(X_iris, y_iris)
assert model.n_iter_ <= 10
model.set_params(max_iter=20)
model.fit(X_iris, y_iris)
assert model.n_iter_ <= 20
```

You can also try `MLPClassifier` which leads to over iterating.

### Expected Results

`n_iter_ <= 20`

### Actual Results

```pytb
    def test_xxx():
        model = MLPRegressor(warm_start=True, early_stopping=False, max_iter=10)
        model.fit(X_iris, y_iris)
        assert model.n_iter_ <= 10
        model.set_params(max_iter=20)
        model.fit(X_iris, y_iris)
>       assert model.n_iter_ <= 20
E       assert 30 <= 20
E        +  where 30 = MLPRegressor(max_iter=20, warm_start=True).n_iter_
```

### Versions

```shell
System:
    python: 3.8.12 | packaged by conda-forge | (default, Sep 16 2021, 01:38:21)  [Clang 11.1.0 ]
executable: /Users/glemaitre/mambaforge/envs/dev/bin/python
   machine: macOS-12.6-arm64-arm-64bit

Python dependencies:
      sklearn: 1.2.dev0
          pip: 21.3
   setuptools: 58.2.0
        numpy: 1.21.6
        scipy: 1.8.0
       Cython: 0.29.24
       pandas: 1.5.0
   matplotlib: 3.4.3
       joblib: 1.2.0
threadpoolctl: 2.2.0

Built with OpenMP: True

threadpoolctl info:
       user_api: blas
   internal_api: openblas
         prefix: libopenblas
       filepath: /Users/glemaitre/mambaforge/envs/dev/lib/libopenblas_vortexp-r0.3.18.dylib
        version: 0.3.18
threading_layer: openmp
   architecture: VORTEX
    num_threads: 8

       user_api: openmp
   internal_api: openmp
         prefix: libomp
       filepath: /Users/glemaitre/mambaforge/envs/dev/lib/libomp.dylib
        version: None
    num_threads: 8
```

Hints from issue discussion:
I would like to investigate this.
Just change the **random_state** parameter to **0** i.e. **random_state=_0_**. This will give you the same result
@Julisam sorry I don't follow.
I think ``max_iter`` should probably be the total number of calls for consistency with ``RandomForest`` (and gradient boosting?). That means if max_iter is reached and you call fit it shouldn't do anything (and maybe give an error?).

Not 100% this is the least unexpected behavior, though.
Note that LBFGS solver works as expected.
OK, the reason is that the `n_iter_` is not reset to `0` in the non-incremental case (calling `fit` instead of `partial_fit`.
Need to check if this is a bug or a feature cf. https://github.com/scikit-learn/scikit-learn/issues/16812
~~But with warm start, aren't you asking for 20 **more** iterations ? (leading to 30 in total)~~

EDIT: actually, thinking more about that I agree that it should be 20 in total (we are not doing incremental learning here)

Commit message: Bug: Change return to break (out of loop)

Patch to review:
diff --git a/sklearn/neural_network/_multilayer_perceptron.py b/sklearn/neural_network/_multilayer_perceptron.py
index 7ed0ab33a0f29..f950e6d5c649c 100644
--- a/sklearn/neural_network/_multilayer_perceptron.py
+++ b/sklearn/neural_network/_multilayer_perceptron.py
@@ -687,6 +687,7 @@ def _fit_stochastic(
                         % self.max_iter,
                         ConvergenceWarning,
                     )
+                    break
         except KeyboardInterrupt:
             warnings.warn("Training interrupted by user.")
 


Code review comments:
diff_hunk: @@ -687,6 +687,7 @@ def _fit_stochastic(
                         % self.max_iter,
                         ConvergenceWarning,
                     )
+                    break
path: sklearn/neural_network/_multilayer_perceptron.py
text: @user1:
Actually, looking at the code, I think it would make more sense to change the for loop from:

```python
for it in range(self.max_iter):
```

to:

```python
for it in range(self.max_iter - self.n_iter_):
```

Basically, it is incorrect for the `for` loop to iterate `max_iter` at each call. We only want to have the difference when warm-starting. It looks more logical to me than breaking the loop.

@user1:
I would even make it explicit:

```python
max_iter_remaining = self.max_iter - self.n_iter_
for it in range(max_iter_remaining):
```

The naming will make it explicit why we subtract `self.n_iter_`

@author:
Hey, thanks for review. I did some changes in the `test_mlp.py` and don't understand why the test `test_learning_rate_warmstart()` divide `mlp.learning_rate_init` through `pow()`. I did the change and it was pass but I can not explain why it works. Have you an answer for me?

The other changes made sense for me, but please have a look.