Talip7/scikit-learn-issues-multilabel huggingface dataset:
HistGradientBoosting avoid data shuffling when early_stopping activated

hello, it would be useful if the _HistGradientBoostingRegressor_ or _HistGradientBoostingClassifier_ model had the ability to avoid data shuffling when using the _early_stopping_ and _validation_fraction_ parameters, since maintaining data order is a basic requirement in case you work with TimeSeries

  https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py#L427
```python
            if sample_weight is None:
                X_train, X_val, y_train, y_val = train_test_split(
                    X,
                    y,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
                sample_weight_train = sample_weight_val = None
            else:
                # TODO: incorporate sample_weight in sampling here, as well as
                # stratify
                (
                    X_train,
                    X_val,
                    y_train,
                    y_val,
                    sample_weight_train,
                    sample_weight_val,
                ) = train_test_split(
                    X,
                    y,
                    sample_weight,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
```


### Describe your proposed solution

it would be sufficient to add an additional parameter to control whether or not to shuffle the data

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
@shamzos as said in my previous message, it is not clear to me what is the way forward. I would wait for the comment of @ogrisel and @jeremiedbb

Labels: module:ensemble, Needs Decision - Include Feature
HistGradientBoosting avoid data shuffling when early_stopping activated

hello, it would be useful if the _HistGradientBoostingRegressor_ or _HistGradientBoostingClassifier_ model had the ability to avoid data shuffling when using the _early_stopping_ and _validation_fraction_ parameters, since maintaining data order is a basic requirement in case you work with TimeSeries

  https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py#L427
```python
            if sample_weight is None:
                X_train, X_val, y_train, y_val = train_test_split(
                    X,
                    y,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
                sample_weight_train = sample_weight_val = None
            else:
                # TODO: incorporate sample_weight in sampling here, as well as
                # stratify
                (
                    X_train,
                    X_val,
                    y_train,
                    y_val,
                    sample_weight_train,
                    sample_weight_val,
                ) = train_test_split(
                    X,
                    y,
                    sample_weight,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
```


### Describe your proposed solution

it would be sufficient to add an additional parameter to control whether or not to shuffle the data

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
The callback API discussed by @glemaitre is being drafted in #22000. This work is paused because it's indeed quite complex to get the API right to allow adding enough flexibility in early stopping data splits while still being intuitive to use, in particular with nesting cross-validation for model selection and evaluation...

Labels: module:ensemble, Needs Decision - Include Feature
HistGradientBoosting avoid data shuffling when early_stopping activated

hello, it would be useful if the _HistGradientBoostingRegressor_ or _HistGradientBoostingClassifier_ model had the ability to avoid data shuffling when using the _early_stopping_ and _validation_fraction_ parameters, since maintaining data order is a basic requirement in case you work with TimeSeries

  https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py#L427
```python
            if sample_weight is None:
                X_train, X_val, y_train, y_val = train_test_split(
                    X,
                    y,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
                sample_weight_train = sample_weight_val = None
            else:
                # TODO: incorporate sample_weight in sampling here, as well as
                # stratify
                (
                    X_train,
                    X_val,
                    y_train,
                    y_val,
                    sample_weight_train,
                    sample_weight_val,
                ) = train_test_split(
                    X,
                    y,
                    sample_weight,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
```


### Describe your proposed solution

it would be sufficient to add an additional parameter to control whether or not to shuffle the data

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
Why not just add the parameters `shuffle` and `stratify` to the estimators and pass them to the `train_test_split`?

Labels: module:ensemble, Needs Decision - Include Feature
HistGradientBoosting avoid data shuffling when early_stopping activated

hello, it would be useful if the _HistGradientBoostingRegressor_ or _HistGradientBoostingClassifier_ model had the ability to avoid data shuffling when using the _early_stopping_ and _validation_fraction_ parameters, since maintaining data order is a basic requirement in case you work with TimeSeries

  https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py#L427
```python
            if sample_weight is None:
                X_train, X_val, y_train, y_val = train_test_split(
                    X,
                    y,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
                sample_weight_train = sample_weight_val = None
            else:
                # TODO: incorporate sample_weight in sampling here, as well as
                # stratify
                (
                    X_train,
                    X_val,
                    y_train,
                    y_val,
                    sample_weight_train,
                    sample_weight_val,
                ) = train_test_split(
                    X,
                    y,
                    sample_weight,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
```


### Describe your proposed solution

it would be sufficient to add an additional parameter to control whether or not to shuffle the data

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
I will submit a PR to introduce a `shuffle` parameter. In that case we can choose to automatically set `stratify=None` if `shuffle=True`.

Labels: module:ensemble, Needs Decision - Include Feature
HistGradientBoosting avoid data shuffling when early_stopping activated

hello, it would be useful if the _HistGradientBoostingRegressor_ or _HistGradientBoostingClassifier_ model had the ability to avoid data shuffling when using the _early_stopping_ and _validation_fraction_ parameters, since maintaining data order is a basic requirement in case you work with TimeSeries

  https://github.com/scikit-learn/scikit-learn/blob/98cf537f5/sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py#L427
```python
            if sample_weight is None:
                X_train, X_val, y_train, y_val = train_test_split(
                    X,
                    y,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
                sample_weight_train = sample_weight_val = None
            else:
                # TODO: incorporate sample_weight in sampling here, as well as
                # stratify
                (
                    X_train,
                    X_val,
                    y_train,
                    y_val,
                    sample_weight_train,
                    sample_weight_val,
                ) = train_test_split(
                    X,
                    y,
                    sample_weight,
                    test_size=self.validation_fraction,
                    stratify=stratify,
                    random_state=self._random_seed,
                )
```


### Describe your proposed solution

it would be sufficient to add an additional parameter to control whether or not to shuffle the data

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
There is a related discussion in #18748 where the conclusion is to add `X_val` and `y_val` as arguments of `fit`. This avoids to add all the splitter options in HGBT where they don’t belong.

Labels: module:ensemble, Needs Decision - Include Feature
Incorrect test test_validation.py::test_cross_validate?

### Describe the bug

I wanted to learn more about how cross_validation is implemented in sklearn and came across what I think is a faulty or confusing test.

The test test_cross_validate in model_selection/tests/test_validation.py creates two datasets, one regression model and one classification model. The test loops over the dataset x model combinations and fits and scores the evaluation metrics. In the loop **est** is used to represent the model under consideration (Lasso or SVC). However, in that process, clone(**reg**).fit(...) is called, so that the Lasso regression is fit and evaluated in both iterations, also for the classification data. If I'm not mistaken, this should be clone(**est**).fit(...), to train and evaluate the SVC classifier. 

However, using clone on **est** actually fails the test, which I did not expect given the comment "It's okay to evaluate regression metrics on classification too".

To me this shows that the test is not testing what it is supposed to be testing.

### Steps/Code to Reproduce

Run the test test_cross_validate()

### Expected Results

The test should use the regressor on the regression data and the classier on the classification data, while passing the test.

### Actual Results

The test uses the Lasso regressor on both the regression data and the classification data, while passing the test.

### Versions

```shell
System:
    python: 3.9.15 | packaged by conda-forge | (main, Nov 22 2022, 08:41:22) [MSC v.1929 64 bit (AMD64)]
executable: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\python.exe
   machine: Windows-10-10.0.19044-SP0
Python dependencies:
      sklearn: 1.3.dev0
          pip: 22.3.1
   setuptools: 66.0.0
        numpy: 1.24.1
        scipy: 1.10.0
       Cython: 0.29.33
       pandas: None
   matplotlib: None
       joblib: 1.2.0
threadpoolctl: 3.1.0
Built with OpenMP: True
threadpoolctl info:
       user_api: blas
   internal_api: mkl
         prefix: libblas
       filepath: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\Library\bin\libblas.dll
        version: 2022.1-Product
threading_layer: intel
    num_threads: 4
       user_api: openmp
   internal_api: openmp
         prefix: vcomp
       filepath: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\vcomp140.dll
        version: None
    num_threads: 8
```

COMMENT:
https://github.com/scikit-learn/scikit-learn/blob/4162ade89a1e2f0c8daacc5816eb2b240607d6a6/sklearn/model_selection/tests/test_validation.py#L429-L435

this is the code referred to above. It does look like a mistake to clone `reg`. Or if not, people from the future would benefit from a comment explaining why it isn't a bug.

Labels: Bug
Incorrect test test_validation.py::test_cross_validate?

### Describe the bug

I wanted to learn more about how cross_validation is implemented in sklearn and came across what I think is a faulty or confusing test.

The test test_cross_validate in model_selection/tests/test_validation.py creates two datasets, one regression model and one classification model. The test loops over the dataset x model combinations and fits and scores the evaluation metrics. In the loop **est** is used to represent the model under consideration (Lasso or SVC). However, in that process, clone(**reg**).fit(...) is called, so that the Lasso regression is fit and evaluated in both iterations, also for the classification data. If I'm not mistaken, this should be clone(**est**).fit(...), to train and evaluate the SVC classifier. 

However, using clone on **est** actually fails the test, which I did not expect given the comment "It's okay to evaluate regression metrics on classification too".

To me this shows that the test is not testing what it is supposed to be testing.

### Steps/Code to Reproduce

Run the test test_cross_validate()

### Expected Results

The test should use the regressor on the regression data and the classier on the classification data, while passing the test.

### Actual Results

The test uses the Lasso regressor on both the regression data and the classification data, while passing the test.

### Versions

```shell
System:
    python: 3.9.15 | packaged by conda-forge | (main, Nov 22 2022, 08:41:22) [MSC v.1929 64 bit (AMD64)]
executable: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\python.exe
   machine: Windows-10-10.0.19044-SP0
Python dependencies:
      sklearn: 1.3.dev0
          pip: 22.3.1
   setuptools: 66.0.0
        numpy: 1.24.1
        scipy: 1.10.0
       Cython: 0.29.33
       pandas: None
   matplotlib: None
       joblib: 1.2.0
threadpoolctl: 3.1.0
Built with OpenMP: True
threadpoolctl info:
       user_api: blas
   internal_api: mkl
         prefix: libblas
       filepath: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\Library\bin\libblas.dll
        version: 2022.1-Product
threading_layer: intel
    num_threads: 4
       user_api: openmp
   internal_api: openmp
         prefix: vcomp
       filepath: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\vcomp140.dll
        version: None
    num_threads: 8
```

COMMENT:
I think the problem is that the `cross_validate()` calls in `check_cross_validate_single_metric` and `check_cross_validate_multi_metric` slit the (very small) dataset differently. So now the results are effected by large statistical fluctuations.

I think passing the `cv` instance created in `test_cross_validate` to those two functions and then passing it on to the `cross_validate` calls would solve the problem.

Do you want to create a Pull Request that implements this?

ps. nice find! Eagle eyed you must be ;)

Labels: Bug
Incorrect test test_validation.py::test_cross_validate?

### Describe the bug

I wanted to learn more about how cross_validation is implemented in sklearn and came across what I think is a faulty or confusing test.

The test test_cross_validate in model_selection/tests/test_validation.py creates two datasets, one regression model and one classification model. The test loops over the dataset x model combinations and fits and scores the evaluation metrics. In the loop **est** is used to represent the model under consideration (Lasso or SVC). However, in that process, clone(**reg**).fit(...) is called, so that the Lasso regression is fit and evaluated in both iterations, also for the classification data. If I'm not mistaken, this should be clone(**est**).fit(...), to train and evaluate the SVC classifier. 

However, using clone on **est** actually fails the test, which I did not expect given the comment "It's okay to evaluate regression metrics on classification too".

To me this shows that the test is not testing what it is supposed to be testing.

### Steps/Code to Reproduce

Run the test test_cross_validate()

### Expected Results

The test should use the regressor on the regression data and the classier on the classification data, while passing the test.

### Actual Results

The test uses the Lasso regressor on both the regression data and the classification data, while passing the test.

### Versions

```shell
System:
    python: 3.9.15 | packaged by conda-forge | (main, Nov 22 2022, 08:41:22) [MSC v.1929 64 bit (AMD64)]
executable: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\python.exe
   machine: Windows-10-10.0.19044-SP0
Python dependencies:
      sklearn: 1.3.dev0
          pip: 22.3.1
   setuptools: 66.0.0
        numpy: 1.24.1
        scipy: 1.10.0
       Cython: 0.29.33
       pandas: None
   matplotlib: None
       joblib: 1.2.0
threadpoolctl: 3.1.0
Built with OpenMP: True
threadpoolctl info:
       user_api: blas
   internal_api: mkl
         prefix: libblas
       filepath: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\Library\bin\libblas.dll
        version: 2022.1-Product
threading_layer: intel
    num_threads: 4
       user_api: openmp
   internal_api: openmp
         prefix: vcomp
       filepath: C:\Users\s.elgewily\Miniconda3\envs\sklearn-env2\vcomp140.dll
        version: None
    num_threads: 8
```

COMMENT:
Thanks for checking and suggestions. I made the changes and added a PR: https://github.com/scikit-learn/scikit-learn/pull/25456

Labels: Bug
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
Searching the file for additional occurrences, a similar situation appears in 9 others situations throughout this file, mostly in the context of β loss calculation, but also generally to avoid devision by zero.
I don't know what is sklearn's general policy regarding adding something to avoid devision by zero, but i suggest maybe working with `finfo.smallest_normal` or something between that and the defined ε (say the geometric average of the two, or the square root of smallest_normal). see [here](https://numpy.org/doc/stable/reference/generated/numpy.finfo.html) for details

Labels: module:decomposition
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
Thanks for the report. Changing to `WH_data[WH_data < EPSILON] = ΕPSILON` seems reasonable, most likely more robust than `WH_data[WH_data == 0] = ΕPSILON`. Do you want to submit a pull-request?

Here, `EPSILON = np.finfo(np.float32).eps`, which is quite standard in scikit-learn. What is the reason you suggest working with `finfo.smallest_normal` instead?

Labels: module:decomposition
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
I can submit a pull request, any specifics i need for the process?

Regarding the ε size, specifically in NMF the WH matrix is the multiplication of two matrices W & H, and it might be the values in the ε range of either one is important, and when they are multiplied the result is going to be in the ε^2 range. I don't think that its critical though.
A more important note is that the behavior in other decomposition modules is slightly different:
`_lda.py` uses `EPS = np.info(float).eps` - that is system dependent,
`_pca.py` uses `eps = 1e-15` - that is not far from `np.finfo(np.float64).eps`
`test_nmf.py` uses `np.finfo(np.float64).eps`
and `_nmf.py` uses `np.finfo(np.float32).eps`
Ideally the choice should be dependent on the datatype of X and WH itself, but as EPSILON is a constant of the module I would suggest going with the decision of the LDA implementation, using `np.info(float).eps`

Labels: module:decomposition
Avoiding zeros in NMF β loss calculation causes inconsistency

https://github.com/scikit-learn/scikit-learn/blob/3f82f840599cc8d2d1278cd4d35aae2089f2e00d/sklearn/decomposition/_nmf.py#L143

The line `WH_data[WH_data == 0] = ΕPSILON` as part of the β loss calculation creates a situation where some entries in the data that were larger than other (say 1E-09 was larger than zero) are now smaller than the other.
I think the code should be corrected to 
`WH_data[WH_data < EPSILON] = ΕPSILON`
to avoid such inconsistencies.
Another issue is preventing overflows in the devision that happens a few lines later - if the data has the smallest positive number (around 10^-38), dividing a number larger than 1 by it causes the overflow.

COMMENT:
Shouldn't `WH_data += ΕPSILON` be more mathematically correct?

Labels: module:decomposition