Talip7/scikit-learn-issues-multilabel huggingface dataset:
CI Issues regarding conda lock files

Opening a new issue regarding some of the discussions around https://github.com/scikit-learn/scikit-learn/pull/27448#issuecomment-1733374337

@lesteve these are maybe what I have in mind:

- they're generated files and usually it's a good idea not to have generated files in the repo
- the files are generated, and their generation depends on the exact time when the conda repo data was downloaded in the user's machine. So there's basically no way to review those changes. Which means we have a ton of changed lines which we cannot review.
- since they're generated and we don't review them, we can't really make sure somebody hasn't manually changed anything in the changed file and that the files are indeed generated and not touched afterwards.
- a significant amount of changed lines has been coming from these files
- we are not even consistent on the use of the same snapshot for different CI. We might be using a generated file from last week for circleCI, and one from a month ago for some of the Azure stuff, and one from two weeks ago for other Azure configs.
- we don't really have good docs on how contributors could use the lock files or if they should. Those are only used in the CI, and for that they don't need to be in the same repo.

A few things we could do:
- only allow changes to those files via PRs done by a bot that we write, and generate them periodically, like weekly maybe, and daily for some which change often (like scipy-dev)
- move them out of the main repo.
- we could also move to a docker based system where our CI doesn't have to download and install for each job for each commit. This would also save us a bunch of time, and it would make it even easer to reproduce CI locally.

COMMENT:
FYI, I bumped into SciTools/iris which seems to have a bot that updates conda-lock lock files, I put more info in https://github.com/scikit-learn/scikit-learn/issues/22425#issuecomment-1739067905

Labels: Build / CI, Needs Decision
CI Issues regarding conda lock files

Opening a new issue regarding some of the discussions around https://github.com/scikit-learn/scikit-learn/pull/27448#issuecomment-1733374337

@lesteve these are maybe what I have in mind:

- they're generated files and usually it's a good idea not to have generated files in the repo
- the files are generated, and their generation depends on the exact time when the conda repo data was downloaded in the user's machine. So there's basically no way to review those changes. Which means we have a ton of changed lines which we cannot review.
- since they're generated and we don't review them, we can't really make sure somebody hasn't manually changed anything in the changed file and that the files are indeed generated and not touched afterwards.
- a significant amount of changed lines has been coming from these files
- we are not even consistent on the use of the same snapshot for different CI. We might be using a generated file from last week for circleCI, and one from a month ago for some of the Azure stuff, and one from two weeks ago for other Azure configs.
- we don't really have good docs on how contributors could use the lock files or if they should. Those are only used in the CI, and for that they don't need to be in the same repo.

A few things we could do:
- only allow changes to those files via PRs done by a bot that we write, and generate them periodically, like weekly maybe, and daily for some which change often (like scipy-dev)
- move them out of the main repo.
- we could also move to a docker based system where our CI doesn't have to download and install for each job for each commit. This would also save us a bunch of time, and it would make it even easer to reproduce CI locally.

COMMENT:
Closing this since we have a bot updating those files now.

Labels: Build / CI, Needs Decision
CI Issues regarding conda lock files

Opening a new issue regarding some of the discussions around https://github.com/scikit-learn/scikit-learn/pull/27448#issuecomment-1733374337

@lesteve these are maybe what I have in mind:

- they're generated files and usually it's a good idea not to have generated files in the repo
- the files are generated, and their generation depends on the exact time when the conda repo data was downloaded in the user's machine. So there's basically no way to review those changes. Which means we have a ton of changed lines which we cannot review.
- since they're generated and we don't review them, we can't really make sure somebody hasn't manually changed anything in the changed file and that the files are indeed generated and not touched afterwards.
- a significant amount of changed lines has been coming from these files
- we are not even consistent on the use of the same snapshot for different CI. We might be using a generated file from last week for circleCI, and one from a month ago for some of the Azure stuff, and one from two weeks ago for other Azure configs.
- we don't really have good docs on how contributors could use the lock files or if they should. Those are only used in the CI, and for that they don't need to be in the same repo.

A few things we could do:
- only allow changes to those files via PRs done by a bot that we write, and generate them periodically, like weekly maybe, and daily for some which change often (like scipy-dev)
- move them out of the main repo.
- we could also move to a docker based system where our CI doesn't have to download and install for each job for each commit. This would also save us a bunch of time, and it would make it even easer to reproduce CI locally.

COMMENT:
Thanks, I was going to write a long and complicated comment saying "what should we do about this" and you saved me some time and effort here :wink:

Labels: Build / CI, Needs Decision
⚠️ CI failed on Linux_nogil.pylatest_pip_nogil ⚠️

**CI failed on [Linux_nogil.pylatest_pip_nogil](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=59484&view=logs&j=67fbb25f-e417-50be-be55-3b1e9637fce5)** (Sep 25, 2023)
- test_pairwise_distances_argkmin[45-csr_matrix-float32-parallel_on_X-cityblock-0-500]
- test_pairwise_distances_argkmin[45-csr_matrix-float32-parallel_on_Y-cityblock-0-500]
- test_pairwise_distances_argkmin[45-csr_array-float32-parallel_on_X-cityblock-0-500]
- test_pairwise_distances_argkmin[45-csr_array-float32-parallel_on_Y-cityblock-0-500]

COMMENT:
Duplicate of #27126 and will be fixed by #27281.

Labels: Needs Triage
Results of `LogisticRegression` are sensitive to the scale of `class_weight`

### Describe the bug

When fitting `LogisticRegression` to a dataset with imbalanced classes, `class_weight` parameter seems to produce different results that depend on the scale of weights, even though the ratio of the weights is the same, e.g., 1/2. This behaviour seems to occur only in some datasets.  An MCVE is provided below.

### Steps/Code to Reproduce

```python
import numpy as np
from sklearn.linear_model import LogisticRegression
from sklearn.metrics import f1_score

N = 100  # number of samples
np.random.seed(0)
y = np.random.rand(N) > .8
X = np.random.rand(N, 10) > .5

for factor in range(1, 6):
    class_weight = {
        False: 1 * factor,
        True: 2 * factor
    }
    clf = LogisticRegression(class_weight=class_weight, random_state=42)
    clf.fit(X, y)
    y_pred = clf.predict(X)
    print(f'Factor: {factor} F1-score: {f1_score(y, y_pred):.3f}' )
```

### Expected Results

I would expect that the `f1_score` is the same and does not depend on the scaling of class weights by a constant factor since the ratio between the classes is still 1/2.

### Actual Results

The actual output I get is this:

```
Factor: 1 F1-score: 0.235
Factor: 2 F1-score: 0.222
Factor: 3 F1-score: 0.316
Factor: 4 F1-score: 0.316
Factor: 5 F1-score: 0.316
```

### Versions

```shell
System:
    python: 3.10.12 | packaged by conda-forge | (main, Jun 23 2023, 22:41:52) [Clang 15.0.7 ]
executable: /Users/alinacherkas/opt/anaconda3/envs/testenv/bin/python
   machine: macOS-13.5.2-arm64-arm-64bit

Python dependencies:
      sklearn: 1.3.0
          pip: 23.2.1
   setuptools: 68.0.0
        numpy: 1.25.2
        scipy: 1.11.1
       Cython: None
       pandas: 2.0.3
   matplotlib: 3.7.1
       joblib: 1.2.0
threadpoolctl: 2.2.0

Built with OpenMP: True

threadpoolctl info:
       filepath: /Users/alinacherkas/opt/anaconda3/envs/testenv/lib/libopenblas.0.dylib
         prefix: libopenblas
       user_api: blas
   internal_api: openblas
        version: 0.3.21
    num_threads: 10
threading_layer: openmp
   architecture: VORTEX

       filepath: /Users/alinacherkas/opt/anaconda3/envs/testenv/lib/libomp.dylib
         prefix: libomp
       user_api: openmp
   internal_api: openmp
        version: None
    num_threads: 10
```

COMMENT:
This is expected behavior. Class weights are multiplied to the sample weights and the optimization problem is (unfortunately) sensible to the scale of the weights (and the number of samples). This dependency is introduced by the (anti-) penalty parameter `C`.

What about introducing a redundant 2. penalty parameter as `1/n * sum(loss) + new_param * penalty`? That would avoid a lot of confusion.

@glemaitre @TomDLT @agramfort @ogrisel @rth What do you think?

Labels: Bug, Needs Triage
Results of `LogisticRegression` are sensitive to the scale of `class_weight`

### Describe the bug

When fitting `LogisticRegression` to a dataset with imbalanced classes, `class_weight` parameter seems to produce different results that depend on the scale of weights, even though the ratio of the weights is the same, e.g., 1/2. This behaviour seems to occur only in some datasets.  An MCVE is provided below.

### Steps/Code to Reproduce

```python
import numpy as np
from sklearn.linear_model import LogisticRegression
from sklearn.metrics import f1_score

N = 100  # number of samples
np.random.seed(0)
y = np.random.rand(N) > .8
X = np.random.rand(N, 10) > .5

for factor in range(1, 6):
    class_weight = {
        False: 1 * factor,
        True: 2 * factor
    }
    clf = LogisticRegression(class_weight=class_weight, random_state=42)
    clf.fit(X, y)
    y_pred = clf.predict(X)
    print(f'Factor: {factor} F1-score: {f1_score(y, y_pred):.3f}' )
```

### Expected Results

I would expect that the `f1_score` is the same and does not depend on the scaling of class weights by a constant factor since the ratio between the classes is still 1/2.

### Actual Results

The actual output I get is this:

```
Factor: 1 F1-score: 0.235
Factor: 2 F1-score: 0.222
Factor: 3 F1-score: 0.316
Factor: 4 F1-score: 0.316
Factor: 5 F1-score: 0.316
```

### Versions

```shell
System:
    python: 3.10.12 | packaged by conda-forge | (main, Jun 23 2023, 22:41:52) [Clang 15.0.7 ]
executable: /Users/alinacherkas/opt/anaconda3/envs/testenv/bin/python
   machine: macOS-13.5.2-arm64-arm-64bit

Python dependencies:
      sklearn: 1.3.0
          pip: 23.2.1
   setuptools: 68.0.0
        numpy: 1.25.2
        scipy: 1.11.1
       Cython: None
       pandas: 2.0.3
   matplotlib: 3.7.1
       joblib: 1.2.0
threadpoolctl: 2.2.0

Built with OpenMP: True

threadpoolctl info:
       filepath: /Users/alinacherkas/opt/anaconda3/envs/testenv/lib/libopenblas.0.dylib
         prefix: libopenblas
       user_api: blas
   internal_api: openblas
        version: 0.3.21
    num_threads: 10
threading_layer: openmp
   architecture: VORTEX

       filepath: /Users/alinacherkas/opt/anaconda3/envs/testenv/lib/libomp.dylib
         prefix: libomp
       user_api: openmp
   internal_api: openmp
        version: None
    num_threads: 10
```

COMMENT:
cf https://github.com/scikit-learn/scikit-learn/blob/2d8e03f4d/sklearn/linear_model/_logistic.py#L323

first I would update https://scikit-learn.org/stable/modules/linear_model.html#binary-case to make explicit how class and sample weights appear in the cost function that is minimized.

are you suggesting to have C or something else to set the regularization? like having C and alpha?

Labels: Bug, Needs Triage
Results of `LogisticRegression` are sensitive to the scale of `class_weight`

### Describe the bug

When fitting `LogisticRegression` to a dataset with imbalanced classes, `class_weight` parameter seems to produce different results that depend on the scale of weights, even though the ratio of the weights is the same, e.g., 1/2. This behaviour seems to occur only in some datasets.  An MCVE is provided below.

### Steps/Code to Reproduce

```python
import numpy as np
from sklearn.linear_model import LogisticRegression
from sklearn.metrics import f1_score

N = 100  # number of samples
np.random.seed(0)
y = np.random.rand(N) > .8
X = np.random.rand(N, 10) > .5

for factor in range(1, 6):
    class_weight = {
        False: 1 * factor,
        True: 2 * factor
    }
    clf = LogisticRegression(class_weight=class_weight, random_state=42)
    clf.fit(X, y)
    y_pred = clf.predict(X)
    print(f'Factor: {factor} F1-score: {f1_score(y, y_pred):.3f}' )
```

### Expected Results

I would expect that the `f1_score` is the same and does not depend on the scaling of class weights by a constant factor since the ratio between the classes is still 1/2.

### Actual Results

The actual output I get is this:

```
Factor: 1 F1-score: 0.235
Factor: 2 F1-score: 0.222
Factor: 3 F1-score: 0.316
Factor: 4 F1-score: 0.316
Factor: 5 F1-score: 0.316
```

### Versions

```shell
System:
    python: 3.10.12 | packaged by conda-forge | (main, Jun 23 2023, 22:41:52) [Clang 15.0.7 ]
executable: /Users/alinacherkas/opt/anaconda3/envs/testenv/bin/python
   machine: macOS-13.5.2-arm64-arm-64bit

Python dependencies:
      sklearn: 1.3.0
          pip: 23.2.1
   setuptools: 68.0.0
        numpy: 1.25.2
        scipy: 1.11.1
       Cython: None
       pandas: 2.0.3
   matplotlib: 3.7.1
       joblib: 1.2.0
threadpoolctl: 2.2.0

Built with OpenMP: True

threadpoolctl info:
       filepath: /Users/alinacherkas/opt/anaconda3/envs/testenv/lib/libopenblas.0.dylib
         prefix: libopenblas
       user_api: blas
   internal_api: openblas
        version: 0.3.21
    num_threads: 10
threading_layer: openmp
   architecture: VORTEX

       filepath: /Users/alinacherkas/opt/anaconda3/envs/testenv/lib/libomp.dylib
         prefix: libomp
       user_api: openmp
   internal_api: openmp
        version: None
    num_threads: 10
```

COMMENT:
> are you suggesting to have C or something else to set the regularization? like having C and alpha?

Yes, I would love to have all linear models use the objective $\frac{1}{\sum_i sw_i} \sum_i sw_i \cdot loss_i + PenaltyStrength \cdot penalty$. The problem is how to get there.

I better open a dedicated issue and close this one.

Labels: Bug, Needs Triage
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
Is this something that used to work but doesn't @riedgar-ms ?

Labels: New Feature
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
I expect it was working circa 2020, since that's when the change to our repo (Fairlearn) which started using `pathlib.Path` was merged. However, the builds are long deleted, so I can't confirm that, or see what version of SciKit-Learn we were pulling.

Labels: New Feature
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
yeah we've added the param validation recently, so this is probably a regression.

Labels: New Feature
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
I think it makes sense to have this option but we should have a proper testing.
If I recall properly, we have a private functions that handles all the `data_home` shared by all fetchers (or at least I hope so) so we should only make sure that it works there.

So some unit testing, updating the documentation and parameter validation should be enough.

Labels: New Feature
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
Can we just directly support all `os.Pathlike`? Currently `get_data_home` already does so.

Labels: New Feature
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
That would probably be okay I think

Labels: New Feature
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
@Charlie-XIAO Indeed, it seems fine with me.

Labels: New Feature
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
Thanks for the confirmation, I'll try to work on this.

Labels: New Feature
Accept pathlib.Path for data_home in fetch_openml

### Describe the workflow you want to enable

When using `fetch_openml()` it would be nice if `pathlib.Path` objects were supported. Currently, there is a type check for `str | None`, so I have to convert my path objects first.

### Describe your proposed solution

Change the accepted type to `pathlib.Path | str | None`

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

_No response_

### Additional context

_No response_

COMMENT:
/take

Labels: New Feature
partial_dependence() with method recursion computes conditional partial dependence for trees

### Describe the bug

For the case of correlated predictors (clearly highly common) the `sklearn.inspection.partial_dependence()` function gives different answers for `method` = "recursion" and `method` = "brute", see my [post](https://markusloecher.github.io/Partial-Dependence-Trees/) for elaborate examples.

I do not believe that this is intentional and should be fixed. Alternatively, it should be communicated clearly in the documentation that (i) the two methods are not equivalent for tree based algorithms, and (ii) that `method` = "recursion" actually computes the **conditional** $E[f(x_S,X_C)|X_S=x_s]$ instead of the (desired) **interventional** $E[f(x_S,X_C)| \mathbf{do}(X_S=x_s)]$ 

### Steps/Code to Reproduce

```python
from sklearn.tree import DecisionTreeRegressor
import numpy as np
import pandas as pd
from sklearn.inspection import PartialDependenceDisplay, partial_dependence

#Generate the data
#first X
N = 400; p0 = 0.5; p11 = 0.8; M=2
X = np.zeros((N,M)) # a matrix (N * M)
N1 = int(p0*N)
X[0:N1,0] = 1
X[0:int(p11*N1),1] = 1
X[N1:(N1+int((1-p11)*(N-N1))+1),1] = 1
df = pd.DataFrame(X, columns=["X0", "X1"])
#then Y
y = np.zeros(N)
y[(X[:,0] == 0) & (X[:,1] == 0)] = 0.3
y[(X[:,0] == 0) & (X[:,1] == 1)] = 0.7
y[(X[:,0] == 1) & (X[:,1] == 0)] = 0.9
y[(X[:,0] == 1) & (X[:,1] == 1)] = 0.1

df = pd.DataFrame(X, columns=["X0", "X1"])
df["y"] = y

model = DecisionTreeRegressor(max_depth=2, max_features = 2)
model.fit(df[["X0","X1"]], y);

features = ["X1"]
for f in features:
    pdp_interventional = partial_dependence(model, df[["X0","X1"]], f,method = "brute")
    pdp_conditional = partial_dependence(model, df[["X0","X1"]], f,method = "recursion")
    
    print(f, "brute (interventional):", pdp_interventional['average'])
    print(f, "recursion (conditional):", pdp_conditional['average'])

pdp_interventional['average'] == pdp_conditional['average']
```

### Expected Results

We would like the two methods to yield the same pdp values, so the last line should yield

`array([[True, True]])`

### Actual Results

Instead they are different. The print statements yield
_X1 brute (interventional): [[0.6 0.4]]
X1 recursion (conditional): [[0.42 0.22]]_
and the last line yields
`array([[False, False]])`

### Versions

```shell
System:
    python: 3.9.0 (v3.9.0:9cf6752276, Oct  5 2020, 11:29:23)  [Clang 6.0 (clang-600.0.57)]
executable: /Users/loecherm/.pyenv/versions/aug_hs_env/bin/python
   machine: macOS-10.16-x86_64-i386-64bit

Python dependencies:
      sklearn: 1.2.0
          pip: 23.0.1
   setuptools: 49.2.1
        numpy: 1.23.5
        scipy: 1.10.0
       Cython: None
       pandas: 1.5.3
   matplotlib: 3.5.3
       joblib: 1.2.0
threadpoolctl: 3.1.0

Built with OpenMP: True
```

COMMENT:
> While I did sketch out a conceptual fix,

If you have a fix for the recursive tree method, even if very rough, we'd be interested! The fix is the problem, the polishing is the easy part.

Labels: Documentation, help wanted