Talip7/scikit-learn-issues-multilabel huggingface dataset:
ArrayAPI: `isdtype` improper comparison to set

### Describe the bug

The newly introduced `sklearn.utils._array_api.isdtype` is comparing types with respect to defined sets.

See here https://github.com/scikit-learn/scikit-learn/blob/cb233674ddffeb1f75f63309f94e4f90d0a2bdf8/sklearn/utils/_array_api.py#L92

This is not working as NumPy's dtype object is "tricky".

### Steps/Code to Reproduce

```python
>>> np.float32 in {np.float64, np.float32}
False
>>> np.float32 in [np.float64, np.float32]
True
```

Because

```python
>>> np.float32 is np.dtype(np.float32)
False
>>> np.float32 == np.dtype(np.float32)
True
```

### Expected Results

The check should be more robust. I suggest using directly `array_api_compat.xp.isdtype` (or vendoring it).

### Actual Results

N/A

### Versions

```shell
On main
```

cc @thomasjpfan

COMMENT:
In any case, I am okay with vendoring the implementation from `array_api_compat.xp.isdtype`.

Labels: Enhancement
Reporting that the EM algorithm didn't converge for a single initialization

### Describe the bug

Should  the code L273-L280
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L273-L280
be indented under the for loop?
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L237

### Steps/Code to Reproduce

Have a look at the bug description.

### Expected Results

We should get a warning for every initialization that fails.

### Actual Results

Currently it does it only for the last initialization as L273-L280 is not indented.

### Versions

```shell
Github.
```

COMMENT:
Hi @krstopro, to me this is the expected behavior. We are running the full EM algorithm ``n_init`` times. In the end we select the best run and only warn if this best run did not converge. This is why the warning part is not indented.

Labels: Bug
Reporting that the EM algorithm didn't converge for a single initialization

### Describe the bug

Should  the code L273-L280
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L273-L280
be indented under the for loop?
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L237

### Steps/Code to Reproduce

Have a look at the bug description.

### Expected Results

We should get a warning for every initialization that fails.

### Actual Results

Currently it does it only for the last initialization as L273-L280 is not indented.

### Versions

```shell
Github.
```

COMMENT:
Thank you for opening the issue @krstopro. I agree with @jeremiedbb and closing this issue.

Labels: Bug
Reporting that the EM algorithm didn't converge for a single initialization

### Describe the bug

Should  the code L273-L280
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L273-L280
be indented under the for loop?
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L237

### Steps/Code to Reproduce

Have a look at the bug description.

### Expected Results

We should get a warning for every initialization that fails.

### Actual Results

Currently it does it only for the last initialization as L273-L280 is not indented.

### Versions

```shell
Github.
```

COMMENT:
@jeremiedbb @thomasjpfan Sorry, I didn't manage to comment before. I know the issue is closed, but I will leave a comment anyway.
The message is not warning the user that the *best* run didn't converge; it is warning that the *last* run didn't converge. The value printed in the message is `init + 1` and `init` is only incremented in the loop. I don't think this is the intended behavior. It would also make sense to warn the user for each run that didn't converge as increasing the number of iterations might outperform the best run that might have converged.
To me it seems that this was the intention of the person that wrote the code, but somehow this part ended up unindented.

Labels: Bug
Reporting that the EM algorithm didn't converge for a single initialization

### Describe the bug

Should  the code L273-L280
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L273-L280
be indented under the for loop?
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L237

### Steps/Code to Reproduce

Have a look at the bug description.

### Expected Results

We should get a warning for every initialization that fails.

### Actual Results

Currently it does it only for the last initialization as L273-L280 is not indented.

### Versions

```shell
Github.
```

COMMENT:
I agree that the reported init number is wrong. We need to track the iteration number of the best run. Would you be interested in submitting a PR for that ?

I still think that the warning at this place is a good compromise as we don't want to flood users with warnings. If a non best run didn't converge it's not that bad to not mention it since the model found a converging run given the budget of iterations anyway.

Labels: Bug
Reporting that the EM algorithm didn't converge for a single initialization

### Describe the bug

Should  the code L273-L280
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L273-L280
be indented under the for loop?
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L237

### Steps/Code to Reproduce

Have a look at the bug description.

### Expected Results

We should get a warning for every initialization that fails.

### Actual Results

Currently it does it only for the last initialization as L273-L280 is not indented.

### Versions

```shell
Github.
```

COMMENT:
@jeremiedbb Thanks for reopening and prompt response!
I agree, a single warning that the best iteration didn't converge might be enough.
Sure, I can submit a PR for that but I might need few days for it. I guess that is fine.

Labels: Bug
Reporting that the EM algorithm didn't converge for a single initialization

### Describe the bug

Should  the code L273-L280
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L273-L280
be indented under the for loop?
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L237

### Steps/Code to Reproduce

Have a look at the bug description.

### Expected Results

We should get a warning for every initialization that fails.

### Actual Results

Currently it does it only for the last initialization as L273-L280 is not indented.

### Versions

```shell
Github.
```

COMMENT:
I like the direction you are going. One thought I had: do we need to tell the user the iteration number of the run in the warning? The way I understand it is that we make several attempts (`n_init`), neither of which converged and as a result we warn the user that the best run didn't converge. I am assuming that a converged run would never be "worse than" a uncoverged run. Hence the thinking that "if the best run didn't converge then none of the runs converged".

All of this makes me think that the user doesn't need to know (and can't really do anything with the information) which of the independent runs was the best and didn't converge. The problem they need to solve is that none of the runs converged. Maybe that makes it easier to write a nice warning and without having to track more information.

Labels: Bug
Reporting that the EM algorithm didn't converge for a single initialization

### Describe the bug

Should  the code L273-L280
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L273-L280
be indented under the for loop?
https://github.com/scikit-learn/scikit-learn/blob/364c77e047ca08a95862becf40a04fe9d4cd2c98/sklearn/mixture/_base.py#L237

### Steps/Code to Reproduce

Have a look at the bug description.

### Expected Results

We should get a warning for every initialization that fails.

### Actual Results

Currently it does it only for the last initialization as L273-L280 is not indented.

### Versions

```shell
Github.
```

COMMENT:
Alright, submitted a PR request!

Now it will warn the user that the best performing initialisation didn't converge. The user can still inspect every individual initialisation by setting `verbose` to 1 or greater.

Labels: Bug
Column-wise parallelism for Column Transformer

### Describe the workflow you want to enable

cc @Vincent-Maladiere 

Currently, when parallelizing the `ColumnTransformer`, it creates 1 job per transformer. This is inefficient as it may create stragglers.

For instance, in the case when there are 20 columns with a slow transformer (e.g. 1min per col) and 20 others with a very fast transformer (e.g. 1 second per col), parallelizing the operation transformer-wise may take 20mins to run.

On the other hand, if we can parallelize column wise, the operation can go down to 1 min.

### Describe your proposed solution

Parallelize the `ColumnTransformer` on each column, instead of each transformer.

According to @GaelVaroquaux, we could [special case the univariate transformers that can be column-wise parallelized](https://github.com/skrub-data/skrub/issues/586#issuecomment-1586308788) (for instance, [Standard Scaler](https://scikit-learn.org/stable/modules/generated/sklearn.preprocessing.StandardScaler.html)).

Multivariate would stay transformer wise parallelized (e.g. [PCA](https://scikit-learn.org/stable/modules/generated/sklearn.decomposition.PCA.html)).

This could be done by introducing transformer tags.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

Discussed with the [issue of parallelizing skrub's TableVectorizer](https://github.com/skrub-data/skrub/issues/586).

COMMENT:
See more discussion at this skrub's PR: https://github.com/skrub-data/skrub/pull/592
An idea: maybe add an option to the Column transformer to either 1) parallelize on the transformers and don't parallelize each transformer (what sklearn does now) or 2) run each transformer sequencially, but parallelize each of them internally. This is not optimal (and adds a new parameter), but it does prevent issues with nested parallelism.

Labels: New Feature
Column-wise parallelism for Column Transformer

### Describe the workflow you want to enable

cc @Vincent-Maladiere 

Currently, when parallelizing the `ColumnTransformer`, it creates 1 job per transformer. This is inefficient as it may create stragglers.

For instance, in the case when there are 20 columns with a slow transformer (e.g. 1min per col) and 20 others with a very fast transformer (e.g. 1 second per col), parallelizing the operation transformer-wise may take 20mins to run.

On the other hand, if we can parallelize column wise, the operation can go down to 1 min.

### Describe your proposed solution

Parallelize the `ColumnTransformer` on each column, instead of each transformer.

According to @GaelVaroquaux, we could [special case the univariate transformers that can be column-wise parallelized](https://github.com/skrub-data/skrub/issues/586#issuecomment-1586308788) (for instance, [Standard Scaler](https://scikit-learn.org/stable/modules/generated/sklearn.preprocessing.StandardScaler.html)).

Multivariate would stay transformer wise parallelized (e.g. [PCA](https://scikit-learn.org/stable/modules/generated/sklearn.decomposition.PCA.html)).

This could be done by introducing transformer tags.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

Discussed with the [issue of parallelizing skrub's TableVectorizer](https://github.com/skrub-data/skrub/issues/586).

COMMENT:
Hey @LeoGrin, we prefer adding a tag to transformers over adding an option to the API. The tags will split Univariate (StandardScaler) from Multivariate (PCA). Univariate will be column-wise parallelized, while Multivariate will stay transformer-wise parallelized (the current default in sklearn).

Labels: New Feature
Column-wise parallelism for Column Transformer

### Describe the workflow you want to enable

cc @Vincent-Maladiere 

Currently, when parallelizing the `ColumnTransformer`, it creates 1 job per transformer. This is inefficient as it may create stragglers.

For instance, in the case when there are 20 columns with a slow transformer (e.g. 1min per col) and 20 others with a very fast transformer (e.g. 1 second per col), parallelizing the operation transformer-wise may take 20mins to run.

On the other hand, if we can parallelize column wise, the operation can go down to 1 min.

### Describe your proposed solution

Parallelize the `ColumnTransformer` on each column, instead of each transformer.

According to @GaelVaroquaux, we could [special case the univariate transformers that can be column-wise parallelized](https://github.com/skrub-data/skrub/issues/586#issuecomment-1586308788) (for instance, [Standard Scaler](https://scikit-learn.org/stable/modules/generated/sklearn.preprocessing.StandardScaler.html)).

Multivariate would stay transformer wise parallelized (e.g. [PCA](https://scikit-learn.org/stable/modules/generated/sklearn.decomposition.PCA.html)).

This could be done by introducing transformer tags.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

Discussed with the [issue of parallelizing skrub's TableVectorizer](https://github.com/skrub-data/skrub/issues/586).

COMMENT:
I understand, but I think which option is the fastest is not obvious for univariate transformers. Quoting @glemaitre:
> To give a concrete example, I think it would be faster to run a StandardScaler on a matrix of several columns instead of paralleling StandardScaler for each column of the matrix.

Labels: New Feature
Column-wise parallelism for Column Transformer

### Describe the workflow you want to enable

cc @Vincent-Maladiere 

Currently, when parallelizing the `ColumnTransformer`, it creates 1 job per transformer. This is inefficient as it may create stragglers.

For instance, in the case when there are 20 columns with a slow transformer (e.g. 1min per col) and 20 others with a very fast transformer (e.g. 1 second per col), parallelizing the operation transformer-wise may take 20mins to run.

On the other hand, if we can parallelize column wise, the operation can go down to 1 min.

### Describe your proposed solution

Parallelize the `ColumnTransformer` on each column, instead of each transformer.

According to @GaelVaroquaux, we could [special case the univariate transformers that can be column-wise parallelized](https://github.com/skrub-data/skrub/issues/586#issuecomment-1586308788) (for instance, [Standard Scaler](https://scikit-learn.org/stable/modules/generated/sklearn.preprocessing.StandardScaler.html)).

Multivariate would stay transformer wise parallelized (e.g. [PCA](https://scikit-learn.org/stable/modules/generated/sklearn.decomposition.PCA.html)).

This could be done by introducing transformer tags.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

Discussed with the [issue of parallelizing skrub's TableVectorizer](https://github.com/skrub-data/skrub/issues/586).

COMMENT:
You're definitely right, users parallelizing StandardScaler will have a slight performance regression. However:
1. If you're using StandardScaler alone, you don't have a good reason to parallelize it column-wise.
2. If you're using StandardScaler with a slow transformer (e.g. [GapEncoder](https://skrub-data.org/stable/generated/skrub.GapEncoder.html)) and want to parallelize them column-wise, the bottleneck will be the slow transformer, not the StandardScaler.

Labels: New Feature
Column-wise parallelism for Column Transformer

### Describe the workflow you want to enable

cc @Vincent-Maladiere 

Currently, when parallelizing the `ColumnTransformer`, it creates 1 job per transformer. This is inefficient as it may create stragglers.

For instance, in the case when there are 20 columns with a slow transformer (e.g. 1min per col) and 20 others with a very fast transformer (e.g. 1 second per col), parallelizing the operation transformer-wise may take 20mins to run.

On the other hand, if we can parallelize column wise, the operation can go down to 1 min.

### Describe your proposed solution

Parallelize the `ColumnTransformer` on each column, instead of each transformer.

According to @GaelVaroquaux, we could [special case the univariate transformers that can be column-wise parallelized](https://github.com/skrub-data/skrub/issues/586#issuecomment-1586308788) (for instance, [Standard Scaler](https://scikit-learn.org/stable/modules/generated/sklearn.preprocessing.StandardScaler.html)).

Multivariate would stay transformer wise parallelized (e.g. [PCA](https://scikit-learn.org/stable/modules/generated/sklearn.decomposition.PCA.html)).

This could be done by introducing transformer tags.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

Discussed with the [issue of parallelizing skrub's TableVectorizer](https://github.com/skrub-data/skrub/issues/586).

COMMENT:
I agree, though I think I should explain more where I'm coming from for my first proposal (i.e summarize quickly https://github.com/skrub-data/skrub/pull/592):
- if you parallelize the transformers by splitting the univariate transformers into multiple transformers (one for each column), it works, but creates a weird `transformers_` object (there might be smart ways to bypass this) (see https://github.com/skrub-data/skrub/pull/592 for some code). 
- otherwise, you might want to use each transformer's inner parallelism. In this case you can
    - do nested parallelism, but it seems hard to make it work properly.
    - favor either the outer loop (what sklearn does now) or the inner loop. In this case, I think it's not obvious what is the right choice, as it depends on the slowness of each transformers, and how many columns are assigned to each tranformer. This is why I was suggesting to add an option, though I'm not sure this is the right choice.

Labels: New Feature
Column-wise parallelism for Column Transformer

### Describe the workflow you want to enable

cc @Vincent-Maladiere 

Currently, when parallelizing the `ColumnTransformer`, it creates 1 job per transformer. This is inefficient as it may create stragglers.

For instance, in the case when there are 20 columns with a slow transformer (e.g. 1min per col) and 20 others with a very fast transformer (e.g. 1 second per col), parallelizing the operation transformer-wise may take 20mins to run.

On the other hand, if we can parallelize column wise, the operation can go down to 1 min.

### Describe your proposed solution

Parallelize the `ColumnTransformer` on each column, instead of each transformer.

According to @GaelVaroquaux, we could [special case the univariate transformers that can be column-wise parallelized](https://github.com/skrub-data/skrub/issues/586#issuecomment-1586308788) (for instance, [Standard Scaler](https://scikit-learn.org/stable/modules/generated/sklearn.preprocessing.StandardScaler.html)).

Multivariate would stay transformer wise parallelized (e.g. [PCA](https://scikit-learn.org/stable/modules/generated/sklearn.decomposition.PCA.html)).

This could be done by introducing transformer tags.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

Discussed with the [issue of parallelizing skrub's TableVectorizer](https://github.com/skrub-data/skrub/issues/586).

COMMENT:
IMHO, the performance boost is worth the UX weirdness. You can still inspect `transformers_` with a for-loop.
cc @GaelVaroquaux which advocates for this solution.

Labels: New Feature
Add Multiclass to Target Encoder

Add multi-class to target encoder, https://github.com/scikit-learn/scikit-learn/pull/26185. @lucyleeow is currently looking into this.

COMMENT:
Implementation wise, I think we follow the method described in 3.3, and **do not** drop any of the classes. Dropping any of the classes will introduce a bias for downstream ML models.

Labels: New Feature, module:preprocessing
DOC Outdated note at top of `build_tools/circle/push_doc.sh`

### Describe the issue linked to the documentation

The top of [`build_tools/circle/push_doc.sh`](https://github.com/scikit-learn/scikit-learn/blob/main/build_tools/circle/push_doc.sh) says:

```
# The behavior of the script is controlled by environment variable defined
# in the circle.yml in the top level folder of the project.
```

I think `circle.yml` has changed to `.circleci/config.yml` although the deloy step doesn't seem to use the `environment` key, so potentially this note could be removed altogether?

Ref: https://github.com/scikit-learn/scikit-learn/issues/26598#issuecomment-1596152204

### Suggest a potential alternative/fix

Amend  `circle.yml`  to `.circleci/config.yml` or remove this part altogether.

COMMENT:
cc @rand0wn

Labels: Documentation, Needs Triage
DOC Outdated note at top of `build_tools/circle/push_doc.sh`

### Describe the issue linked to the documentation

The top of [`build_tools/circle/push_doc.sh`](https://github.com/scikit-learn/scikit-learn/blob/main/build_tools/circle/push_doc.sh) says:

```
# The behavior of the script is controlled by environment variable defined
# in the circle.yml in the top level folder of the project.
```

I think `circle.yml` has changed to `.circleci/config.yml` although the deloy step doesn't seem to use the `environment` key, so potentially this note could be removed altogether?

Ref: https://github.com/scikit-learn/scikit-learn/issues/26598#issuecomment-1596152204

### Suggest a potential alternative/fix

Amend  `circle.yml`  to `.circleci/config.yml` or remove this part altogether.

COMMENT:
Thanks @lucyleeow for creating the issue, I can see a $CIRCLE_BRANCH variable in the deploy section of config.yml which could be the environment variable the doc is referring to, so made the changes accordingly.

Labels: Documentation, Needs Triage
DOC Outdated note at top of `build_tools/circle/push_doc.sh`

### Describe the issue linked to the documentation

The top of [`build_tools/circle/push_doc.sh`](https://github.com/scikit-learn/scikit-learn/blob/main/build_tools/circle/push_doc.sh) says:

```
# The behavior of the script is controlled by environment variable defined
# in the circle.yml in the top level folder of the project.
```

I think `circle.yml` has changed to `.circleci/config.yml` although the deloy step doesn't seem to use the `environment` key, so potentially this note could be removed altogether?

Ref: https://github.com/scikit-learn/scikit-learn/issues/26598#issuecomment-1596152204

### Suggest a potential alternative/fix

Amend  `circle.yml`  to `.circleci/config.yml` or remove this part altogether.

COMMENT:
I think that part just checks the git branch and only runs the deploy script if it is on `main` branch. Im not sure if that's what is meant by controlling the "behaviour" of the script.

Labels: Documentation, Needs Triage