Talip7/scikit-learn-issues-multilabel huggingface dataset:
Configure OpenBLAS to use scikit-learn's OpenMP threadpool

OpenBLAS v0.3.28 will have a new feature allowing OpenBLAS to use the threadpool chosen by the user, (see https://github.com/OpenMathLib/OpenBLAS/pull/4577).

This is very interesting because it would solve a performance issue happening when there's a quick succession of BLAS calls and OpenMP (prange) calls. The issue happens when OpenBLAS and OpenMP don't share the same threadpool because both threadpools are in active wait mode when they're idle (see https://github.com/OpenMathLib/OpenBLAS/issues/3187 for details), which is a current situation since numpy and scipy wheels are built against OpenBLAS with the pthreads threading layer.

This issue is currently impacting some estimators like KMeans (https://github.com/scikit-learn/scikit-learn/issues/20642), NMF (https://github.com/scikit-learn/scikit-learn/pull/16439), pairwise_distances (https://github.com/scikit-learn/scikit-learn/issues/26097), ...

Being able to configure OpenBLAS to use our OpenMP threadpool would allow to get rid of this issue even if numpy and scipy keep building their wheels against OpenBLAS pthreads (which is very likely).

I'm not sure yet if or how https://github.com/OpenMathLib/OpenBLAS/pull/4577 would make this possible so I'm opening this issue to track the progress on this subject.

COMMENT:
I am not sure we can do that as part of `threadpoolctl` itself or more precisely, if we do it as part of threadpoolctl, then openblas will use the openmp runtime linked against a native extension shipped with `threadpoolctl` but we would have no guarantee that this is the same runtime as the one linked to `sklearn`'s Cython native extensions with `prange` loops.

Labels: Performance
Configure OpenBLAS to use scikit-learn's OpenMP threadpool

OpenBLAS v0.3.28 will have a new feature allowing OpenBLAS to use the threadpool chosen by the user, (see https://github.com/OpenMathLib/OpenBLAS/pull/4577).

This is very interesting because it would solve a performance issue happening when there's a quick succession of BLAS calls and OpenMP (prange) calls. The issue happens when OpenBLAS and OpenMP don't share the same threadpool because both threadpools are in active wait mode when they're idle (see https://github.com/OpenMathLib/OpenBLAS/issues/3187 for details), which is a current situation since numpy and scipy wheels are built against OpenBLAS with the pthreads threading layer.

This issue is currently impacting some estimators like KMeans (https://github.com/scikit-learn/scikit-learn/issues/20642), NMF (https://github.com/scikit-learn/scikit-learn/pull/16439), pairwise_distances (https://github.com/scikit-learn/scikit-learn/issues/26097), ...

Being able to configure OpenBLAS to use our OpenMP threadpool would allow to get rid of this issue even if numpy and scipy keep building their wheels against OpenBLAS pthreads (which is very likely).

I'm not sure yet if or how https://github.com/OpenMathLib/OpenBLAS/pull/4577 would make this possible so I'm opening this issue to track the progress on this subject.

COMMENT:
>  if we do it as part of threadpoolctl, then openblas will use the openmp runtime linked against a native extension shipped with threadpoolctl

I do not think `threadpoolctl` should ship any runtimes.

I was thinking of using `threadpoolctl` to connect the openmp runtime loaded by `sklearn` with `OpenBLAS`. Concretely, Scikit-learn is responsible for calling `threadpoolctl.register_openblas_backend("openmp")`. If we want to be more verbose:

```python
PATH_TO_LIBOMP = ".../lib/python3.12/site-packages/sklearn/.dylibs/libomp.dylib"
threadpoolctl.register_openblas_backend(PATH_TO_LIBOMP)
```

Labels: Performance
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
In principle `sample_weight` can be accessed when fitting `TargetEncoder`. So at first sight this is something that can be considered as a new feature. There seems to be some support for it as well (👍  on the issue).

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
Related to #11316 for testing.

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
I am working on this.

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
Hi! I am working on this issue with @MiguelParece. We have already incorporated sample weights to compute the weighted averages, but we are currently uncertain about whether we should also use them to calculate the variance when` smooth="auto"`. I believe using sample weights for variance calculation is the standard approach, but we would like to confirm if this is indeed the best practice before proceeding with the implementation and testing.

@mayer79 @betatim

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
> Hi! I am working on this issue with @MiguelParece. We have already incorporated sample weights to compute the weighted averages, but we are currently uncertain about whether we should also use them to calculate the variance when` smooth="auto"`. I believe using sample weights for variance calculation is the standard approach, but we would like to confirm if this is indeed the best practice before proceeding with the implementation and testing.
> 
> @mayer79 @betatim

Good point - I'd go for a weighted variance. Now, there are different ways to calculate weighted variances, see e.g. https://numpy.org/doc/stable/reference/generated/numpy.cov.html, which is related to the meaning of the "weights":

- frequency weights: a value 2 means "the row represents two observations" or
- general weights, where a value 2.5 could mean: "the observation is very important"

What type of weights should we stick to @lorentzenchr ?

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
> > Hi! I am working on this issue with @MiguelParece. We have already incorporated sample weights to compute the weighted averages, but we are currently uncertain about whether we should also use them to calculate the variance when` smooth="auto"`. I believe using sample weights for variance calculation is the standard approach, but we would like to confirm if this is indeed the best practice before proceeding with the implementation and testing.
> > @mayer79 @betatim
> 
> Good point - I'd go for a weighted variance. Now, there are different ways to calculate weighted variances, see e.g. https://numpy.org/doc/stable/reference/generated/numpy.cov.html, which is related to the meaning of the "weights":
> 
>     * frequency weights: a value 2 means "the row represents two observations" or
> 
>     * general weights, where a value 2.5 could mean: "the observation is very important"
> 
> 
> What type of weights should we stick to @lorentzenchr ?


From what I understand from [this comment](https://github.com/scikit-learn/scikit-learn/issues/15836#issuecomment-1994181871) on another issue regarding `sample_weight` we should use frequency weights. Would you agree @mayer79 ?

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
In the end, it should not matter that much. I prefer the general weights as those are met more often in ML and freq weights are just more special (like in a survey).

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
Agree @lorentzenchr . Maybe mention in the docstring of the new argument which formula is used to calculate weighted variance.

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
`check_sample_weight_equivalence_on_dense_data` which is part of our common test suite checks for the frequency weights semantics. I think this is what we should strive for in general but if there are cases where those semantics are not reachable for a particular reason, this reason should be made explicit in the XFAIL message of that particular estimator:

https://github.com/scikit-learn/scikit-learn/blob/0dbbac961ee251309aa7ec6deee658da83b0eaf0/sklearn/utils/_test_common/instance_generator.py#L851

Labels: New Feature
`TargetEncoder` should respect `sample_weights`

### Describe the workflow you want to enable

The current implementation of `TargetEncoder` seems to calculate (shrinked) averages of `y`. In cases with `sample_weights`, it would be more natural to work with (shrinked) weighted averages.

### Describe your proposed solution

In case of `sample_weights`, shrinked averages should be replaced by corresponding shrinked weighted averages.

However, I am not 100% sure if `sample_weights` are accessable by a transformer.

### Describe alternatives you've considered, if relevant

The alternative is to continue ignoring sample weights.

### Additional context

_No response_

COMMENT:
Related PR / discussion: https://github.com/scikit-learn/scikit-learn/pull/30564

Labels: New Feature
⚠️ CI failed on macOS.pylatest_conda_forge_mkl (last failure: Apr 24, 2024) ⚠️

**CI failed on [macOS.pylatest_conda_forge_mkl](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=66022&view=logs&j=97641769-79fb-5590-9088-a30ce9b850b9)** (Apr 24, 2024)
- test_neighbors_metrics[float32-minkowski]

COMMENT:
## CI is no longer failing! ✅

[Successful run](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=66083&view=logs&j=97641769-79fb-5590-9088-a30ce9b850b9) on Apr 26, 2024

Labels: Numerical Stability
⚠️ CI failed on macOS.pylatest_conda_mkl_no_openmp (last failure: Apr 20, 2024) ⚠️

**CI failed on [macOS.pylatest_conda_mkl_no_openmp](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=65958&view=logs&j=e6d5b7c0-0dfd-5ddf-13d5-c71bebf56ce2)** (Apr 20, 2024)
Unable to find junit file. Please see link for details.

COMMENT:
## CI is no longer failing! ✅

[Successful run](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=65968&view=logs&j=e6d5b7c0-0dfd-5ddf-13d5-c71bebf56ce2) on Apr 22, 2024

Labels: Needs Triage
⚠️ CI failed on macOS.pylatest_conda_mkl_no_openmp (last failure: Apr 20, 2024) ⚠️

**CI failed on [macOS.pylatest_conda_mkl_no_openmp](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=65958&view=logs&j=e6d5b7c0-0dfd-5ddf-13d5-c71bebf56ce2)** (Apr 20, 2024)
Unable to find junit file. Please see link for details.

COMMENT:
This is odd. It's passing now, so closing the issue, but for the record:

```
  FAILED: sklearn/neighbors/_kd_tree.cpython-312-darwin.so.p/sklearn/neighbors/_kd_tree.pyx.c
  cython -M --fast-fail -3 '-X language_level=3' '-X boundscheck=False' '-X wraparound=False' '-X initializedcheck=False' '-X nonecheck=False' '-X cdivision=True' '-X profile=False' --include-dir /Users/runner/work/1/s/build/cp312 sklearn/neighbors/_kd_tree.pyx -o sklearn/neighbors/_kd_tree.cpython-312-darwin.so.p/sklearn/neighbors/_kd_tree.pyx.c

  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  from libc.string cimport memcpy

  import numpy as np
  import warnings

  from ..metrics._dist_metrics cimport (
  ^
  ------------------------------------------------------------

  sklearn/neighbors/_binary_tree.pxi:148:0: 'sklearn/metrics/_dist_metrics.pxd' not found
  [81/249] Compiling Cython source sklearn/_loss/_loss.pyx
  [82/249] Compiling Cython source /Users/runner/work/1/s/sklearn/preprocessing/_csr_polynomial_expansion.pyx
  [83/249] Compiling Cython source /Users/runner/work/1/s/sklearn/neighbors/_quad_tree.pyx
  ninja: build stopped: subcommand failed.
```

cc @lesteve

Labels: Needs Triage
⚠️ CI failed on macOS.pylatest_conda_mkl_no_openmp (last failure: Apr 20, 2024) ⚠️

**CI failed on [macOS.pylatest_conda_mkl_no_openmp](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=65958&view=logs&j=e6d5b7c0-0dfd-5ddf-13d5-c71bebf56ce2)** (Apr 20, 2024)
Unable to find junit file. Please see link for details.

COMMENT:
This is another instance of https://github.com/scikit-learn/scikit-learn/issues/28820

Labels: Needs Triage
⚠️ CI failed on Ubuntu_Jammy_Jellyfish.pymin_conda_forge_openblas_ubuntu_2204 (last failure: Apr 19, 2024) ⚠️

**CI failed on [Ubuntu_Jammy_Jellyfish.pymin_conda_forge_openblas_ubuntu_2204](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=65919&view=logs&j=f71949a9-f9d9-549e-cf45-2e99c7b412d1)** (Apr 19, 2024)
- test_pca_sparse[32-1-arpack-csr_matrix-10-0.01]
- test_pca_sparse[32-1-arpack-csr_array-10-0.01]
- test_pca_sparse[32-1-arpack-csc_matrix-10-0.01]
- test_pca_sparse[32-1-arpack-csc_array-10-0.01]

COMMENT:
Has this test been brittle before? Have you seen it pop up @lesteve @jeremiedbb maybe?

Labels: module:decomposition
⚠️ CI failed on Ubuntu_Jammy_Jellyfish.pymin_conda_forge_openblas_ubuntu_2204 (last failure: Apr 19, 2024) ⚠️

**CI failed on [Ubuntu_Jammy_Jellyfish.pymin_conda_forge_openblas_ubuntu_2204](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=65919&view=logs&j=f71949a9-f9d9-549e-cf45-2e99c7b412d1)** (Apr 19, 2024)
- test_pca_sparse[32-1-arpack-csr_matrix-10-0.01]
- test_pca_sparse[32-1-arpack-csr_array-10-0.01]
- test_pca_sparse[32-1-arpack-csc_matrix-10-0.01]
- test_pca_sparse[32-1-arpack-csc_array-10-0.01]

COMMENT:
## CI is no longer failing! ✅

[Successful run](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=66083&view=logs&j=f71949a9-f9d9-549e-cf45-2e99c7b412d1) on Apr 26, 2024

Labels: module:decomposition
Make it possible to specify `monotonic_cst` with feature names in all tree-based estimators

### Describe the workflow you want to enable

Instead of passing an array of monotonicity constraints (-1 for a decrease constraint, +1 for an increase constraint or 0 for no constraint) specified by feature positions in the training set, it would be more convenient to pass a dict to pass constraints spec only for the required feature names. For instance
``` python
from sklearn.datasets import load_diabetes
from sklearn.ensemble import RandomForestRegressor

X, y = load_diabetes(return_X_y=True, as_frame=True)

reg = RandomForestRegressor(
    monotonic_cst={"bmi": +1, "s3": -1}
)
reg.fit(X, y)
```
Not that here X has column names because it is a `pd.DataFrame`.

Note that this already supported for `HistGradientBoostingRegressor`. Ideally this would be supported across all tree-based models for consistency.

### Describe your proposed solution

Use the `_check_monotonic_cst` function to validate the `monotonic_cst` argument in all estimators.

### Describe alternatives you've considered, if relevant

This has already been implemented for `HistGradientBoostingRegressor ` in #24855.

### Additional context

See #24855 for the implementation of this for `HistGradientBoostingRegressor`.

COMMENT:
I don't think we'd be adding this for all tree based models. But we'll be moving HistGradientBoosting under general GradientBoosting most probably. (https://github.com/scikit-learn/scikit-learn/issues/27873)

cc @adam2392 @lorentzenchr @glemaitre

Labels: New Feature, Needs Triage
mean_squred_error giving wrong results

### Describe the bug

I have recently noticed a bug in the implementation of mean_squared_error in sklearn.metrics.
The current implementation of the function basically calculates the MSE as follows: 
```
output_errors = np.average((y_true - y_pred) ** 2, axis=0, weights=sample_weight)
```
Which is reasonable in most cases, but may return wrong results in cases that the type of `y_true` and `y_pred` has a low bit count, for example `np.uint8` ranging from 0 to 254. 
The reason for that is that when doing the calculation using arrays of types like `np.uint8`, it is very likely that overflows will occur (which are not reported in any way!) resulting in wrong results.
To resolve this `y_true` and `y_pred` should first be casted to a `dtype` big enough so overflows will not occur with reasonable errors, such as `float64`.
For example:
```
def mse(image_1:np.ndarray, image_2:np.ndarray) -> float:
    return (np.square(image_1.astype(np.float64)-image_2.astype(np.float64))).mean()
```

### Steps/Code to Reproduce

```
import numpy as np
from sklearn.metrics import mean_squared_error

true = np.array([0], dtype=np.uint8)
pred = np.array([16], dtype=np.uint8)
mean_squared_error(true, pred)
```

### Expected Results

Expected result is 256 as (0 - 16)**2 = 256

### Actual Results

The result of mean_squared_error is  0

### Versions

```shell
System:
    python: 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]
executable: /usr/bin/python3
   machine: Linux-6.1.58+-x86_64-with-glibc2.35

Python dependencies:
      sklearn: 1.4.2
          pip: 23.1.2
   setuptools: 67.7.2
        numpy: 1.25.2
        scipy: 1.11.4
       Cython: 3.0.10
       pandas: 2.0.3
   matplotlib: 3.7.1
       joblib: 1.4.0
threadpoolctl: 3.4.0

Built with OpenMP: True

threadpoolctl info:
       user_api: blas
   internal_api: openblas
    num_threads: 2
         prefix: libopenblas
       filepath: /usr/local/lib/python3.10/dist-packages/numpy.libs/libopenblas64_p-r0-5007b62f.3.23.dev.so
        version: 0.3.23.dev
threading_layer: pthreads
   architecture: Haswell

       user_api: openmp
   internal_api: openmp
    num_threads: 2
         prefix: libgomp
       filepath: /usr/local/lib/python3.10/dist-packages/scikit_learn.libs/libgomp-a34b3233.so.1.0.0
        version: None

       user_api: blas
   internal_api: openblas
    num_threads: 2
         prefix: libopenblas
       filepath: /usr/local/lib/python3.10/dist-packages/scipy.libs/libopenblasp-r0-23e5df77.3.21.dev.so
        version: 0.3.21.dev
threading_layer: pthreads
   architecture: Haswell
```

COMMENT:
One could argue that about any dtype, since overflow can happen in any of them. So I think it's the user who needs to know what dtype makes sense for such an operation. Otherwise we'd need to upcast every input in every method to the highest available dtype / precision, and we can't do that.

cc @ogrisel @betatim since this is similar to the case we had for the array API.

Closing for now, happy to have it reopened if we think we should do something about it.

Labels: Bug, Needs Triage