Talip7/scikit-learn-issues-multilabel huggingface dataset:
Correctly document linked libraries

### Describe the issue linked to the documentation

When downloading the current wheel for `scikit-learn==1.3.1`, the metadata tell me that the package is subject to the terms of BSD-3-Clause. Unfortunately, this only applies to the package itself. Skimming through the distributed files, there are at least two additional cases:

* External code snippets under licenses like MIT, Apache-2.0 and Python-2.0
* Binary modules like `libgomp-a34b3233.so.1.0.0`, subject to GPL-3.0-or-later WITH Runtime exception: https://github.com/gcc-mirror/gcc/blob/master/libgomp/libgomp.h


### Suggest a potential alternative/fix

It would be great if a full list of external modules shipped within *scikit-learn* wheels and their copyright information would be provided to detect possible license conflicts early.

COMMENT:
@thomasjpfan Could you contribute something similar to https://github.com/numpy/numpy/pull/20102 concerning the licenses?

Labels: Documentation
Correctly document linked libraries

### Describe the issue linked to the documentation

When downloading the current wheel for `scikit-learn==1.3.1`, the metadata tell me that the package is subject to the terms of BSD-3-Clause. Unfortunately, this only applies to the package itself. Skimming through the distributed files, there are at least two additional cases:

* External code snippets under licenses like MIT, Apache-2.0 and Python-2.0
* Binary modules like `libgomp-a34b3233.so.1.0.0`, subject to GPL-3.0-or-later WITH Runtime exception: https://github.com/gcc-mirror/gcc/blob/master/libgomp/libgomp.h


### Suggest a potential alternative/fix

It would be great if a full list of external modules shipped within *scikit-learn* wheels and their copyright information would be provided to detect possible license conflicts early.

COMMENT:
Yea, I'll contribute something like https://github.com/numpy/numpy/pull/20102 for scikit-learn.

Labels: Documentation
Correctly document linked libraries

### Describe the issue linked to the documentation

When downloading the current wheel for `scikit-learn==1.3.1`, the metadata tell me that the package is subject to the terms of BSD-3-Clause. Unfortunately, this only applies to the package itself. Skimming through the distributed files, there are at least two additional cases:

* External code snippets under licenses like MIT, Apache-2.0 and Python-2.0
* Binary modules like `libgomp-a34b3233.so.1.0.0`, subject to GPL-3.0-or-later WITH Runtime exception: https://github.com/gcc-mirror/gcc/blob/master/libgomp/libgomp.h


### Suggest a potential alternative/fix

It would be great if a full list of external modules shipped within *scikit-learn* wheels and their copyright information would be provided to detect possible license conflicts early.

COMMENT:
Anything to contribute here ?

Labels: Documentation
Louvain community detection fails to recognize sparse matrix instance

### Describe the bug

TypeError being thrown by sknetwork/utils/check.py:130, in check_format(input_matrix, allow_empty)

I don't think this should be happening.

### Steps/Code to Reproduce

```python
from sknetwork.clustering import Louvain, get_modularity
import networkx as nx

G = nx.Graph(np.array([[0,1,1],[1,0,0],[1,0,0]]))
A = nx.to_scipy_sparse_array(G)

louvain = Louvain()
louvain.fit(A.tocsr())
```

### Expected Results

No error.

### Actual Results

```
File ~/anaconda3/envs/corp2/lib/python3.10/site-packages/sknetwork/utils/check.py:130, in check_format(input_matrix, allow_empty)
    128 formats = {sparse.csr_matrix, sparse.csc_matrix, sparse.coo_matrix, sparse.lil_matrix, np.ndarray}
    129 if type(input_matrix) not in formats:
--> 130     raise TypeError('The input matrix must be in Scipy sparse format or Numpy ndarray format.')
    131 input_matrix = sparse.csr_matrix(input_matrix)
    132 if not allow_empty and input_matrix.nnz == 0:

TypeError: The input matrix must be in Scipy sparse format or Numpy ndarray format.
```

### Versions

```shell
System:
    python: 3.10.12 | packaged by conda-forge | (main, Jun 23 2023, 22:40:32) [GCC 12.3.0]
executable: /home/eddie/anaconda3/envs/corp2/bin/python
   machine: Linux-6.2.0-33-generic-x86_64-with-glibc2.35

Python dependencies:
      sklearn: 1.3.0
          pip: 23.2.1
   setuptools: 68.2.2
        numpy: 1.26.0
        scipy: 1.11.2
       Cython: None
       pandas: 2.1.0
   matplotlib: 3.7.2
       joblib: 1.3.2
threadpoolctl: 3.2.0

Built with OpenMP: True

threadpoolctl info:
       user_api: blas
   internal_api: openblas
    num_threads: 32
         prefix: libopenblas
       filepath: /home/eddie/anaconda3/envs/corp2/lib/libopenblasp-r0.3.24.so
        version: 0.3.24
threading_layer: pthreads
   architecture: Zen

       user_api: openmp
   internal_api: openmp
    num_threads: 32
         prefix: libomp
       filepath: /home/eddie/anaconda3/envs/corp2/lib/libomp.so
        version: None
```

COMMENT:
sounds like an sknetwork issue and not an sklearn issue.

Labels: Bug, Needs Triage
Modified huber - Bug in the formula

### Describe the issue linked to the documentation


https://scikit-learn.org/stable/modules/sgd.html#mathematical-formulation
1.5.8. Mathematical formulation -> Loss function details -> Modified huber loss

The equation written for huber loss contains a bug. it is written as y_i f(x_i) >1
It should be y_i f(x_i) >-1
![image](https://github.com/scikit-learn/scikit-learn/assets/51014931/70c5d81c-ba17-458a-b263-e79f46468bba)

### Suggest a potential alternative/fix

In documentation, it is written as y_i f(x_i) >1
It should be y_i f(x_i) >-1

COMMENT:
Indeed it should `-1` there. @rahulsm27 do you want to open a pull-request to fix this issue.

Labels: Documentation
Modified huber - Bug in the formula

### Describe the issue linked to the documentation


https://scikit-learn.org/stable/modules/sgd.html#mathematical-formulation
1.5.8. Mathematical formulation -> Loss function details -> Modified huber loss

The equation written for huber loss contains a bug. it is written as y_i f(x_i) >1
It should be y_i f(x_i) >-1
![image](https://github.com/scikit-learn/scikit-learn/assets/51014931/70c5d81c-ba17-458a-b263-e79f46468bba)

### Suggest a potential alternative/fix

In documentation, it is written as y_i f(x_i) >1
It should be y_i f(x_i) >-1

COMMENT:
Yes.

On Mon, Oct 9, 2023 at 1:25 PM Guillaume Lemaitre ***@***.***>
wrote:

> Indeed it should -1 there. @rahulsm27 <https://github.com/rahulsm27> do
> you want to open a pull-request to fix this issue.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/27547#issuecomment-1752507230>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AMFG2E722PS4TYQYV4TJQS3X6OUV5AVCNFSM6AAAAAA5XOA6NSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJSGUYDOMRTGA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


-- 
Thanks and Regards,
Rahul Mahajan
9833045093

Labels: Documentation
Modified huber - Bug in the formula

### Describe the issue linked to the documentation


https://scikit-learn.org/stable/modules/sgd.html#mathematical-formulation
1.5.8. Mathematical formulation -> Loss function details -> Modified huber loss

The equation written for huber loss contains a bug. it is written as y_i f(x_i) >1
It should be y_i f(x_i) >-1
![image](https://github.com/scikit-learn/scikit-learn/assets/51014931/70c5d81c-ba17-458a-b263-e79f46468bba)

### Suggest a potential alternative/fix

In documentation, it is written as y_i f(x_i) >1
It should be y_i f(x_i) >-1

COMMENT:
Hi. Kindly review Pull request #27553

Labels: Documentation
⚠️ CI failed on Ubuntu_Atlas.ubuntu_atlas ⚠️

**CI is still failing on [Ubuntu_Atlas.ubuntu_atlas](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=60142&view=logs&j=689a1c8f-ff4e-5689-1a1a-6fa551ae9eba)** (Oct 18, 2023)
- test_logistic_regressioncv_class_weights[65-balanced-weight1]

COMMENT:
## CI is no longer failing! ✅

[Successful run](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=60174&view=logs&j=689a1c8f-ff4e-5689-1a1a-6fa551ae9eba) on Oct 19, 2023

Labels: Needs Triage
⚠️ CI failed on Ubuntu_Atlas.ubuntu_atlas ⚠️

**CI is still failing on [Ubuntu_Atlas.ubuntu_atlas](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=60142&view=logs&j=689a1c8f-ff4e-5689-1a1a-6fa551ae9eba)** (Oct 18, 2023)
- test_logistic_regressioncv_class_weights[65-balanced-weight1]

COMMENT:
Closing as the CI is no longer failing.

Labels: Needs Triage
SelectKBest shouldn't raise if k > n_samples

### Describe the workflow you want to enable

Let's say I want to build a logistic regression model with at most 50 features. I could do that with something like this: 
``make_pipeline(ColumnTransformer(...OneHotEncoder(), remainder="passthrough"), SelectKBest(k=50), LogisticRegression())``
but whether that estimator succeeds depends on the number of levels in the categorical variables, which is a bit strange. It would be great if ``SelectKBest`` wouldn't error, or at least optionally wouldn't error.

### Describe your proposed solution

I'm undecided between adding an option or changing the behavior. The easy thing would be to add an option but adding an option to disable an error is a bit strange. So ... a deprecation cycle for the error? that either means waiting for the deprecation to go through or adding a temporary variable and then deprecating that.

If someone expects the estimator to create exactly 50 features, then the error is reasonable, but I'm not sure if that's a good interpretation of what ``SelectKBest`` does?

### Describe alternatives you've considered, if relevant

In a pipeline like the above, there's no work-around as far as I can see.

### Additional context

_No response_

COMMENT:
I would not be opposed to raise a warning instead of the error mentioning that all available features are returned.

Labels: New Feature
SelectKBest shouldn't raise if k > n_samples

### Describe the workflow you want to enable

Let's say I want to build a logistic regression model with at most 50 features. I could do that with something like this: 
``make_pipeline(ColumnTransformer(...OneHotEncoder(), remainder="passthrough"), SelectKBest(k=50), LogisticRegression())``
but whether that estimator succeeds depends on the number of levels in the categorical variables, which is a bit strange. It would be great if ``SelectKBest`` wouldn't error, or at least optionally wouldn't error.

### Describe your proposed solution

I'm undecided between adding an option or changing the behavior. The easy thing would be to add an option but adding an option to disable an error is a bit strange. So ... a deprecation cycle for the error? that either means waiting for the deprecation to go through or adding a temporary variable and then deprecating that.

If someone expects the estimator to create exactly 50 features, then the error is reasonable, but I'm not sure if that's a good interpretation of what ``SelectKBest`` does?

### Describe alternatives you've considered, if relevant

In a pipeline like the above, there's no work-around as far as I can see.

### Additional context

_No response_

COMMENT:
That would work for me, as the warning would be expected I could filter it out here.

Labels: New Feature
SelectKBest shouldn't raise if k > n_samples

### Describe the workflow you want to enable

Let's say I want to build a logistic regression model with at most 50 features. I could do that with something like this: 
``make_pipeline(ColumnTransformer(...OneHotEncoder(), remainder="passthrough"), SelectKBest(k=50), LogisticRegression())``
but whether that estimator succeeds depends on the number of levels in the categorical variables, which is a bit strange. It would be great if ``SelectKBest`` wouldn't error, or at least optionally wouldn't error.

### Describe your proposed solution

I'm undecided between adding an option or changing the behavior. The easy thing would be to add an option but adding an option to disable an error is a bit strange. So ... a deprecation cycle for the error? that either means waiting for the deprecation to go through or adding a temporary variable and then deprecating that.

If someone expects the estimator to create exactly 50 features, then the error is reasonable, but I'm not sure if that's a good interpretation of what ``SelectKBest`` does?

### Describe alternatives you've considered, if relevant

In a pipeline like the above, there's no work-around as far as I can see.

### Additional context

_No response_

COMMENT:
To make sure I understand the case properly: you have a dataset where the number of features passed to `SelectKBest` is <=50? And it happens because you one hot encode a feature so the number of featurse ends up depending on the number of unique values in that feature.

Having a warning in the case where `SelectKBest` selects all the features seems reasonable. 

Can you link me to the code where the exception is raised? I was looking at https://github.com/scikit-learn/scikit-learn/blob/d99b728b3a7952b2111cf5e0cb5d14f92c6f3a80/sklearn/feature_selection/_base.py#L24 and can only find where a warning is raised if no features are selected. So there is precedent for using a warning.

Alternatively, some code that reproduces the problem?

Labels: New Feature
Use float32_t for tree.threshold

The features `X` in our standard decision trees are float32, so it would make sense for the threshold of features to also be float32, see https://github.com/scikit-learn/scikit-learn/blob/8ae5f186986667bc3042a36f5d23e352acc40154/sklearn/tree/_tree.pxd#L31

Note that the Cython trees are expose in our trees, e.g. [`DecisionTreeRegressor`](https://scikit-learn.org/stable/modules/generated/sklearn.tree.DecisionTreeRegressor.html#sklearn.tree.DecisionTreeRegressor`) attribute `tree_`.

COMMENT:
This makes sense to me and would help reduce the memory/disc-storage footprint of the tree. 

As a heavy-user of the tree submodule, I don't see an issue in backwards compatibility as even if I saved a tree from earlier sklearn versions, when traversing the tree with data, one is comparing `float64` vs `float32` anyways and any differences would be entirely due to machine precision.

The only issue I can see is if a user explicitly uses the `tree_.threshold` property directly.

Labels: Performance, Needs Decision, module:tree, Breaking Change, cython
Use float32_t for tree.threshold

The features `X` in our standard decision trees are float32, so it would make sense for the threshold of features to also be float32, see https://github.com/scikit-learn/scikit-learn/blob/8ae5f186986667bc3042a36f5d23e352acc40154/sklearn/tree/_tree.pxd#L31

Note that the Cython trees are expose in our trees, e.g. [`DecisionTreeRegressor`](https://scikit-learn.org/stable/modules/generated/sklearn.tree.DecisionTreeRegressor.html#sklearn.tree.DecisionTreeRegressor`) attribute `tree_`.

COMMENT:
> The only issue I can see is if a user explicitly uses the tree_.threshold property directly.

I would say even then one gets the right answer with a float32, it's just less precise. But in truth a float64 has just more precision than the threshold is accurate.

Labels: Performance, Needs Decision, module:tree, Breaking Change, cython
Use float32_t for tree.threshold

The features `X` in our standard decision trees are float32, so it would make sense for the threshold of features to also be float32, see https://github.com/scikit-learn/scikit-learn/blob/8ae5f186986667bc3042a36f5d23e352acc40154/sklearn/tree/_tree.pxd#L31

Note that the Cython trees are expose in our trees, e.g. [`DecisionTreeRegressor`](https://scikit-learn.org/stable/modules/generated/sklearn.tree.DecisionTreeRegressor.html#sklearn.tree.DecisionTreeRegressor`) attribute `tree_`.

COMMENT:
I meant that if they use the threshold directly outside sklearn code for whatever reason and it involves comparing to something with float64 precision then changing 'threshold' to float32 would break their workflow. 

I agree that float32 comparison in general is less precision but as your issue raised 'threshold' is mainly used in sklearn code to compare to X which is converted to float32 anyways. 

Maybe a path forward is just having a deprecation warning within the access of tree_.threshold and then in v1.5 actually change it to float32?

Labels: Performance, Needs Decision, module:tree, Breaking Change, cython
Use float32_t for tree.threshold

The features `X` in our standard decision trees are float32, so it would make sense for the threshold of features to also be float32, see https://github.com/scikit-learn/scikit-learn/blob/8ae5f186986667bc3042a36f5d23e352acc40154/sklearn/tree/_tree.pxd#L31

Note that the Cython trees are expose in our trees, e.g. [`DecisionTreeRegressor`](https://scikit-learn.org/stable/modules/generated/sklearn.tree.DecisionTreeRegressor.html#sklearn.tree.DecisionTreeRegressor`) attribute `tree_`.

COMMENT:
float32 and float64 are pretty interoperable even in C on most operations, comparisons included.

Labels: Performance, Needs Decision, module:tree, Breaking Change, cython
Use float32_t for tree.threshold

The features `X` in our standard decision trees are float32, so it would make sense for the threshold of features to also be float32, see https://github.com/scikit-learn/scikit-learn/blob/8ae5f186986667bc3042a36f5d23e352acc40154/sklearn/tree/_tree.pxd#L31

Note that the Cython trees are expose in our trees, e.g. [`DecisionTreeRegressor`](https://scikit-learn.org/stable/modules/generated/sklearn.tree.DecisionTreeRegressor.html#sklearn.tree.DecisionTreeRegressor`) attribute `tree_`.

COMMENT:
SG! I'm in favor of switching the `threshold` type to float32_t :)

Labels: Performance, Needs Decision, module:tree, Breaking Change, cython
Better inference of the columns remainder dtype in `transformers_` from `ColumnTransformer`

A typical use case is to fit a `ColumnTransfomrer` on a pandas dataframe such as:

```python
# %%
from sklearn.datasets import load_iris

df, y = load_iris(return_X_y=True, as_frame=True)

# %%
from sklearn.preprocessing import StandardScaler
from sklearn.compose import ColumnTransformer

cols = ["sepal length (cm)", "sepal width (cm)"]
ct = ColumnTransformer([("scale", StandardScaler(), cols)], remainder="passthrough")
ct.fit_transform(df)
```

While investigating the `transformers_`, the columns exposed for `remainder` is a bit weird:

```python
ct.transformers_
```

```
[('scale', StandardScaler(), ['sepal length (cm)', 'sepal width (cm)']),
 ('remainder', 'passthrough', [2, 3])]
```

In terms of UX, I assume that one would expect to get the feature names in the remainder instead of the feature indices. However, our API to pass the information of the columns in scikit-learn is pretty flexible: we can have missed type of slice, array of int, etc.

I would suggest that when the columns passed by the user are of a single type, then we make sure that the column type of the `remainder` is of the same type: if only indices are passed, we show indices, if names are passed, we show names, and if boolean are passed, we show boolean.

With mixed types, we cannot really decide and we can keep the current behaviour. On a UX perspective, I assume this is better (together with the change in #27204).

Any thoughts @lorentzenchr @adrinjalali @ogrisel @betatim @jeremiedbb

COMMENT:
+1 for the proposed change. Seems very reasonable to me.

Labels: Enhancement
Better inference of the columns remainder dtype in `transformers_` from `ColumnTransformer`

A typical use case is to fit a `ColumnTransfomrer` on a pandas dataframe such as:

```python
# %%
from sklearn.datasets import load_iris

df, y = load_iris(return_X_y=True, as_frame=True)

# %%
from sklearn.preprocessing import StandardScaler
from sklearn.compose import ColumnTransformer

cols = ["sepal length (cm)", "sepal width (cm)"]
ct = ColumnTransformer([("scale", StandardScaler(), cols)], remainder="passthrough")
ct.fit_transform(df)
```

While investigating the `transformers_`, the columns exposed for `remainder` is a bit weird:

```python
ct.transformers_
```

```
[('scale', StandardScaler(), ['sepal length (cm)', 'sepal width (cm)']),
 ('remainder', 'passthrough', [2, 3])]
```

In terms of UX, I assume that one would expect to get the feature names in the remainder instead of the feature indices. However, our API to pass the information of the columns in scikit-learn is pretty flexible: we can have missed type of slice, array of int, etc.

I would suggest that when the columns passed by the user are of a single type, then we make sure that the column type of the `remainder` is of the same type: if only indices are passed, we show indices, if names are passed, we show names, and if boolean are passed, we show boolean.

With mixed types, we cannot really decide and we can keep the current behaviour. On a UX perspective, I assume this is better (together with the change in #27204).

Any thoughts @lorentzenchr @adrinjalali @ogrisel @betatim @jeremiedbb

COMMENT:
also +1 here.

Labels: Enhancement
Better inference of the columns remainder dtype in `transformers_` from `ColumnTransformer`

A typical use case is to fit a `ColumnTransfomrer` on a pandas dataframe such as:

```python
# %%
from sklearn.datasets import load_iris

df, y = load_iris(return_X_y=True, as_frame=True)

# %%
from sklearn.preprocessing import StandardScaler
from sklearn.compose import ColumnTransformer

cols = ["sepal length (cm)", "sepal width (cm)"]
ct = ColumnTransformer([("scale", StandardScaler(), cols)], remainder="passthrough")
ct.fit_transform(df)
```

While investigating the `transformers_`, the columns exposed for `remainder` is a bit weird:

```python
ct.transformers_
```

```
[('scale', StandardScaler(), ['sepal length (cm)', 'sepal width (cm)']),
 ('remainder', 'passthrough', [2, 3])]
```

In terms of UX, I assume that one would expect to get the feature names in the remainder instead of the feature indices. However, our API to pass the information of the columns in scikit-learn is pretty flexible: we can have missed type of slice, array of int, etc.

I would suggest that when the columns passed by the user are of a single type, then we make sure that the column type of the `remainder` is of the same type: if only indices are passed, we show indices, if names are passed, we show names, and if boolean are passed, we show boolean.

With mixed types, we cannot really decide and we can keep the current behaviour. On a UX perspective, I assume this is better (together with the change in #27204).

Any thoughts @lorentzenchr @adrinjalali @ogrisel @betatim @jeremiedbb

COMMENT:
> When the inputs all use callables for their columns, above I had the remainder use integer indices rather than a callable, too. We could have it be a callable, in this case I guess we would need to check the types that the other callables return, and if they all return the same type (ie int, str, bool), use that as well for the remainder. I'm not sure if it's worth the added complexity, WDYT?

Indices is OK for callable I think.

Also, as this change might break some user code, should there be a deprecation cycle? And if so, does that mean we need to add a parameter that controls whether we apply the old or the new behavior?

Yep, I would prefer to have a gentle change of API.

Labels: Enhancement