Talip7/scikit-learn-issues-multilabel huggingface dataset:
SimpleImputer does not drop a column full of `np.nan` even when `keep_empty_feature=False`

The following code snippet lead to some surprises:

```python
import numpy as np
from sklearn.datasets import load_iris
from sklearn.impute import SimpleImputer

X, y = load_iris(return_X_y=True)
X[:, 0] = np.nan

imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
X_trans = imputer.fit_transform(X)

assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"
```

```pytb
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[19], line 11
      8 imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
      9 X_trans = imputer.fit_transform(X)
---> 11 assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"

AssertionError: X_trans contains 4 columns
```

Apparently this is something that we really wanted for backward compatibility when merging https://github.com/scikit-learn/scikit-learn/issues/24770:

https://github.com/scikit-learn/scikit-learn/blob/c91528c4c2efecc344622e3435157ba3b39a7253/sklearn/impute/tests/test_impute.py#L1670-L1692

Now, I'm wondering if we should not deprecate this behaviour since the parameter `keep_empty_feature` allows to control whether or not we should drop the feature entirely.

So I would propose to warn for a change of behaviour when `strategy="constant"`, `keep_empty_feature=False`, and that we detect that we have empty feature(s).

COMMENT:
It is kind of weird to have this behavior only for the constant strategy. I can't remember the motivation behind that and reading the original PR doesn't make it clearer.

> So I would propose to warn for a change of behaviour when strategy="constant", keep_empty_feature=False, and that we detect that we have empty feature(s).

I'm okay with that.

To go further, I'm wondering if we really want to keep the possibility to remove empty columns. It's been the source of many issues, especially when wrapped in a cross val. I feel like this should be handled separately. We don't have this option for any other preprocessor for instance.

Labels: Bug, API
SimpleImputer does not drop a column full of `np.nan` even when `keep_empty_feature=False`

The following code snippet lead to some surprises:

```python
import numpy as np
from sklearn.datasets import load_iris
from sklearn.impute import SimpleImputer

X, y = load_iris(return_X_y=True)
X[:, 0] = np.nan

imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
X_trans = imputer.fit_transform(X)

assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"
```

```pytb
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[19], line 11
      8 imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
      9 X_trans = imputer.fit_transform(X)
---> 11 assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"

AssertionError: X_trans contains 4 columns
```

Apparently this is something that we really wanted for backward compatibility when merging https://github.com/scikit-learn/scikit-learn/issues/24770:

https://github.com/scikit-learn/scikit-learn/blob/c91528c4c2efecc344622e3435157ba3b39a7253/sklearn/impute/tests/test_impute.py#L1670-L1692

Now, I'm wondering if we should not deprecate this behaviour since the parameter `keep_empty_feature` allows to control whether or not we should drop the feature entirely.

So I would propose to warn for a change of behaviour when `strategy="constant"`, `keep_empty_feature=False`, and that we detect that we have empty feature(s).

COMMENT:
I have come across another issue with  `keep_empty_feature` in #29451 (open PR waiting for reviews, just linking for reference).
 `keep_empty_feature` is present in `SimpleImputer`, `KNNImputer` and `IterativeImputer` since version 1.2.

Labels: Bug, API
SimpleImputer does not drop a column full of `np.nan` even when `keep_empty_feature=False`

The following code snippet lead to some surprises:

```python
import numpy as np
from sklearn.datasets import load_iris
from sklearn.impute import SimpleImputer

X, y = load_iris(return_X_y=True)
X[:, 0] = np.nan

imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
X_trans = imputer.fit_transform(X)

assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"
```

```pytb
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[19], line 11
      8 imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
      9 X_trans = imputer.fit_transform(X)
---> 11 assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"

AssertionError: X_trans contains 4 columns
```

Apparently this is something that we really wanted for backward compatibility when merging https://github.com/scikit-learn/scikit-learn/issues/24770:

https://github.com/scikit-learn/scikit-learn/blob/c91528c4c2efecc344622e3435157ba3b39a7253/sklearn/impute/tests/test_impute.py#L1670-L1692

Now, I'm wondering if we should not deprecate this behaviour since the parameter `keep_empty_feature` allows to control whether or not we should drop the feature entirely.

So I would propose to warn for a change of behaviour when `strategy="constant"`, `keep_empty_feature=False`, and that we detect that we have empty feature(s).

COMMENT:
Thanks @StefanieSenger I'll give a look at this PR.

> To go further, I'm wondering if we really want to keep the possibility to remove empty columns. It's been the source of many issues, especially when wrapped in a cross val. I feel like this should be handled separately. We don't have this option for any other preprocessor for instance.

Now that we have the parameter this is maybe not worth to remove it. However, it might be wise to change the default to `True` to not step into the issue that you mentioned in cross-validation.

Labels: Bug, API
SimpleImputer does not drop a column full of `np.nan` even when `keep_empty_feature=False`

The following code snippet lead to some surprises:

```python
import numpy as np
from sklearn.datasets import load_iris
from sklearn.impute import SimpleImputer

X, y = load_iris(return_X_y=True)
X[:, 0] = np.nan

imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
X_trans = imputer.fit_transform(X)

assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"
```

```pytb
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[19], line 11
      8 imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
      9 X_trans = imputer.fit_transform(X)
---> 11 assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"

AssertionError: X_trans contains 4 columns
```

Apparently this is something that we really wanted for backward compatibility when merging https://github.com/scikit-learn/scikit-learn/issues/24770:

https://github.com/scikit-learn/scikit-learn/blob/c91528c4c2efecc344622e3435157ba3b39a7253/sklearn/impute/tests/test_impute.py#L1670-L1692

Now, I'm wondering if we should not deprecate this behaviour since the parameter `keep_empty_feature` allows to control whether or not we should drop the feature entirely.

So I would propose to warn for a change of behaviour when `strategy="constant"`, `keep_empty_feature=False`, and that we detect that we have empty feature(s).

COMMENT:
import numpy as np
from sklearn.datasets import load_iris
from sklearn.impute import SimpleImputer

# Load the iris dataset
X, y = load_iris(return_X_y=True)

# Introduce NaNs in the first feature
X[:, 0] = np.nan

# Initialize the imputer with constant strategy
imputer = SimpleImputer(strategy="constant", fill_value=1)

# Transform the data
X_trans = imputer.fit_transform(X)

# Check the shape of the transformed data
assert X_trans.shape[1] == X.shape[1], f"X_trans contains {X_trans.shape[1]} columns, expected {X.shape[1]} columns"

print(f"Shape of X_trans: {X_trans.shape}")

Labels: Bug, API
SimpleImputer does not drop a column full of `np.nan` even when `keep_empty_feature=False`

The following code snippet lead to some surprises:

```python
import numpy as np
from sklearn.datasets import load_iris
from sklearn.impute import SimpleImputer

X, y = load_iris(return_X_y=True)
X[:, 0] = np.nan

imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
X_trans = imputer.fit_transform(X)

assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"
```

```pytb
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Cell In[19], line 11
      8 imputer = SimpleImputer(keep_empty_features=False, strategy="constant", fill_value=1)
      9 X_trans = imputer.fit_transform(X)
---> 11 assert X_trans.shape[1] == 3, f"X_trans contains {X.shape[1]} columns"

AssertionError: X_trans contains 4 columns
```

Apparently this is something that we really wanted for backward compatibility when merging https://github.com/scikit-learn/scikit-learn/issues/24770:

https://github.com/scikit-learn/scikit-learn/blob/c91528c4c2efecc344622e3435157ba3b39a7253/sklearn/impute/tests/test_impute.py#L1670-L1692

Now, I'm wondering if we should not deprecate this behaviour since the parameter `keep_empty_feature` allows to control whether or not we should drop the feature entirely.

So I would propose to warn for a change of behaviour when `strategy="constant"`, `keep_empty_feature=False`, and that we detect that we have empty feature(s).

COMMENT:
I am going to work on this issue.

Labels: Bug, API
Misleading variable name for the example of  AUC calculation

### Describe the issue linked to the documentation

In the [example of AUC calculation](https://scikit-learn.org/stable/modules/generated/sklearn.metrics.auc.html), it was given that:

```python
import numpy as np
from sklearn import metrics
y = np.array([1, 1, 2, 2])
pred = np.array([0.1, 0.4, 0.35, 0.8])
fpr, tpr, thresholds = metrics.roc_curve(y, pred, pos_label=2)
metrics.auc(fpr, tpr)
```

Readers will assume that `pred` is the prediction value. In fact, it should be the prediction probabilities, as required by `roc_curve`.


### Suggest a potential alternative/fix

Instead of using `y` and `pred`, giving the same name as required by [`roc_curve`](https://scikit-learn.org/stable/modules/generated/sklearn.metrics.roc_curve.html) would be helpful.

```python
import numpy as np
from sklearn import metrics
y_true = np.array([1, 1, 2, 2])
y_score = np.array([0.1, 0.4, 0.35, 0.8])
fpr, tpr, thresholds = metrics.roc_curve(y_true, y_score, pos_label=2)
metrics.auc(fpr, tpr)
```

COMMENT:
In the same way, both argument and example of `RocCurveDisplay.from_predictions` is also misleading:
Code:  

https://github.com/scikit-learn/scikit-learn/blob/8a2d5ffa0512873a75da608eb14832253979ec44/sklearn/metrics/_plot/roc_curve.py#L284-L295 

and 

https://github.com/scikit-learn/scikit-learn/blob/8a2d5ffa0512873a75da608eb14832253979ec44/sklearn/metrics/_plot/roc_curve.py#L298-L311

Example:
```python
import matplotlib.pyplot as plt
from sklearn.datasets import make_classification
from sklearn.metrics import RocCurveDisplay
from sklearn.model_selection import train_test_split
from sklearn.svm import SVC
X, y = make_classification(random_state=0)
X_train, X_test, y_train, y_test = train_test_split(
    X, y, random_state=0)
clf = SVC(random_state=0).fit(X_train, y_train)
y_pred = clf.decision_function(X_test)
RocCurveDisplay.from_predictions(
   y_test, y_pred)
plt.show()
```

In all cases above `y_pred` should be `y_score`.

Labels: Documentation
Misleading variable name for the example of  AUC calculation

### Describe the issue linked to the documentation

In the [example of AUC calculation](https://scikit-learn.org/stable/modules/generated/sklearn.metrics.auc.html), it was given that:

```python
import numpy as np
from sklearn import metrics
y = np.array([1, 1, 2, 2])
pred = np.array([0.1, 0.4, 0.35, 0.8])
fpr, tpr, thresholds = metrics.roc_curve(y, pred, pos_label=2)
metrics.auc(fpr, tpr)
```

Readers will assume that `pred` is the prediction value. In fact, it should be the prediction probabilities, as required by `roc_curve`.


### Suggest a potential alternative/fix

Instead of using `y` and `pred`, giving the same name as required by [`roc_curve`](https://scikit-learn.org/stable/modules/generated/sklearn.metrics.roc_curve.html) would be helpful.

```python
import numpy as np
from sklearn import metrics
y_true = np.array([1, 1, 2, 2])
y_score = np.array([0.1, 0.4, 0.35, 0.8])
fpr, tpr, thresholds = metrics.roc_curve(y_true, y_score, pos_label=2)
metrics.auc(fpr, tpr)
```

COMMENT:
Hi @bagustris,

Regarding the first remark, i.e. the auc example, I agree that using the same names as the roc_curve arg names would be better. Would you be interested in making a PR to fix it ?

Regarding the arg names of the `RocCurveDisplay.from_predictions` method, I also think that using `y_score` would be more consistent. Although not a major issue imo because the parameter description is clear that probability estimates or decision scores are expected. This change would requires a bit more work because it needs a deprecation cycle with a warning during a 2 releases window to rename the parameter. @glemaitre what do you think ?

Labels: Documentation
Misleading variable name for the example of  AUC calculation

### Describe the issue linked to the documentation

In the [example of AUC calculation](https://scikit-learn.org/stable/modules/generated/sklearn.metrics.auc.html), it was given that:

```python
import numpy as np
from sklearn import metrics
y = np.array([1, 1, 2, 2])
pred = np.array([0.1, 0.4, 0.35, 0.8])
fpr, tpr, thresholds = metrics.roc_curve(y, pred, pos_label=2)
metrics.auc(fpr, tpr)
```

Readers will assume that `pred` is the prediction value. In fact, it should be the prediction probabilities, as required by `roc_curve`.


### Suggest a potential alternative/fix

Instead of using `y` and `pred`, giving the same name as required by [`roc_curve`](https://scikit-learn.org/stable/modules/generated/sklearn.metrics.roc_curve.html) would be helpful.

```python
import numpy as np
from sklearn import metrics
y_true = np.array([1, 1, 2, 2])
y_score = np.array([0.1, 0.4, 0.35, 0.8])
fpr, tpr, thresholds = metrics.roc_curve(y_true, y_score, pos_label=2)
metrics.auc(fpr, tpr)
```

COMMENT:
> Regarding the arg names of the RocCurveDisplay.from_predictions method, I also think that using y_score would be more consistent. Although not a major issue imo because the parameter description is clear that probability estimates or decision scores are expected. This change would requires a bit more work because it needs a deprecation cycle with a warning during a 2 releases window to rename the parameter. @glemaitre what do you think ?

Yep apparently this is something that has been overlooked when we introduce this method. This is not aligned with the `roc_curve` as well. Since that most people will pass the two first variable as positional I think that the swap will not be that much annoying.

So we would need to have the signature `.from_predictions(y_true, y_score, *, ..., y_pred="deprecated")` for the deprecation cycle.

Labels: Documentation
Misleading variable name for the example of  AUC calculation

### Describe the issue linked to the documentation

In the [example of AUC calculation](https://scikit-learn.org/stable/modules/generated/sklearn.metrics.auc.html), it was given that:

```python
import numpy as np
from sklearn import metrics
y = np.array([1, 1, 2, 2])
pred = np.array([0.1, 0.4, 0.35, 0.8])
fpr, tpr, thresholds = metrics.roc_curve(y, pred, pos_label=2)
metrics.auc(fpr, tpr)
```

Readers will assume that `pred` is the prediction value. In fact, it should be the prediction probabilities, as required by `roc_curve`.


### Suggest a potential alternative/fix

Instead of using `y` and `pred`, giving the same name as required by [`roc_curve`](https://scikit-learn.org/stable/modules/generated/sklearn.metrics.roc_curve.html) would be helpful.

```python
import numpy as np
from sklearn import metrics
y_true = np.array([1, 1, 2, 2])
y_score = np.array([0.1, 0.4, 0.35, 0.8])
fpr, tpr, thresholds = metrics.roc_curve(y_true, y_score, pos_label=2)
metrics.auc(fpr, tpr)
```

COMMENT:
> Hi @bagustris,
> 
> Regarding the first remark, i.e., the AUC example, I agree that using the same names as the roc_curve arg names would be better. Would you be interested in making a PR to fix it?
> 
Yes, I am interested in tackling this issue. I'll try to submit PR; please guide me then.

Labels: Documentation
make_regression always generates positive coefficients

### Describe the workflow you want to enable


This is my first issue, please forgive the non-standard format.
I noticed that when using make_regression to generate random data, I always get positive coefficients.I read the source code and found that this situation may be caused by this code.
```
    ground_truth = np.zeros((n_features, n_targets))
    ground_truth[:n_informative, :] = 100 * generator.uniform(
        size=(n_informative, n_targets)
    )

    y = np.dot(X, ground_truth) + bias
```
[make_regression_source](https://github.com/scikit-learn/scikit-learn/blob/8a2d5ffa0/sklearn/datasets/_samples_generator.py#L572)
line708-713

It generates coefficients using 100*U(0,1)


### Describe your proposed solution


In order to be able to generate positive and negative coefficients, modification of the value range can be considered
```
ground_truth = np.zeros((n_features, n_targets))
ground_truth[:n_informative, :] = 100 * generator.uniform(
    low=-1,high=1,
    size=(n_informative, n_targets)
)
y = np.dot(X, ground_truth) + bias
```


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

_No response_

### Additional context

_No response_

COMMENT:
Hi @KyleBao-Fd,
What's wrong with having only positive coefficients ? Is it a real limitation for you ?
We can implement the change you're proposing, but it would require adding a new parameter to the function to switch between old a new behavior, because we don't want to change the behavior without noticing 2 releases ahead. So I'm wondering if the added maintenance burden is worth.

Labels: New Feature
make_regression always generates positive coefficients

### Describe the workflow you want to enable


This is my first issue, please forgive the non-standard format.
I noticed that when using make_regression to generate random data, I always get positive coefficients.I read the source code and found that this situation may be caused by this code.
```
    ground_truth = np.zeros((n_features, n_targets))
    ground_truth[:n_informative, :] = 100 * generator.uniform(
        size=(n_informative, n_targets)
    )

    y = np.dot(X, ground_truth) + bias
```
[make_regression_source](https://github.com/scikit-learn/scikit-learn/blob/8a2d5ffa0/sklearn/datasets/_samples_generator.py#L572)
line708-713

It generates coefficients using 100*U(0,1)


### Describe your proposed solution


In order to be able to generate positive and negative coefficients, modification of the value range can be considered
```
ground_truth = np.zeros((n_features, n_targets))
ground_truth[:n_informative, :] = 100 * generator.uniform(
    low=-1,high=1,
    size=(n_informative, n_targets)
)
y = np.dot(X, ground_truth) + bias
```


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

_No response_

### Additional context

_No response_

COMMENT:
> Hi @KyleBao-Fd, What's wrong with having only positive coefficients ? Is it a real limitation for you ? We can implement the change you're proposing, but it would require adding a new parameter to the function to switch between old a new behavior, because we don't want to change the behavior without noticing 2 releases ahead. So I'm wondering if the added maintenance burden is worth.


I was trying to draw a graph to illustrate the changes in each coefficient as the hyperparameters change.Having only positive coefficients makes my image look a bit weird.
This is not a huge problem, but I think we can solve it without adding parameters as long as we set the generation of positive and negative coefficients by U(-1,1) as the default.I don't think this should cause new problems.
The above are my suggestions, thank you for your reply

Labels: New Feature
make_regression always generates positive coefficients

### Describe the workflow you want to enable


This is my first issue, please forgive the non-standard format.
I noticed that when using make_regression to generate random data, I always get positive coefficients.I read the source code and found that this situation may be caused by this code.
```
    ground_truth = np.zeros((n_features, n_targets))
    ground_truth[:n_informative, :] = 100 * generator.uniform(
        size=(n_informative, n_targets)
    )

    y = np.dot(X, ground_truth) + bias
```
[make_regression_source](https://github.com/scikit-learn/scikit-learn/blob/8a2d5ffa0/sklearn/datasets/_samples_generator.py#L572)
line708-713

It generates coefficients using 100*U(0,1)


### Describe your proposed solution


In order to be able to generate positive and negative coefficients, modification of the value range can be considered
```
ground_truth = np.zeros((n_features, n_targets))
ground_truth[:n_informative, :] = 100 * generator.uniform(
    low=-1,high=1,
    size=(n_informative, n_targets)
)
y = np.dot(X, ground_truth) + bias
```


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

_No response_

### Additional context

_No response_

COMMENT:
> Having only positive coefficients makes my image look a bit weird.

If it's only a visual inconvenience, I'm not sure the change of behavior is worth. What do you think @glemaitre ?

> as long as we set the generation of positive and negative coefficients by U(-1,1) as the default.I don't think this should cause new problems.

The thing is that we want to have backward compatibility between releases and changing the behavior breaks that. That's why it would involve a new parameter and/or a deprecation cycle.

Labels: New Feature
DOC: Add Bioconductor's package to the list of scikit-learn Related Projects

### Describe the issue linked to the documentation

### Description
Add Bioconductor's package to the list of scikit-learn Related Projects:
https://scikit-learn.org/stable/related_projects.html

https://bioconductor.org/packages/release/bioc/html/BiocSklearn.html

---

Hello @vjcitn & @almahmoud,  
Would you like to submit the PR for this one?


### Suggest a potential alternative/fix

_No response_

COMMENT:
Thanks @reshamas I will do it.

Labels: Documentation