Talip7/scikit-learn-issues-multilabel huggingface dataset:
RFC Trigger a copy when copy=False and X is read-only

Highly related to #14481 and maybe a little bit to https://github.com/scikit-learn/scikit-learn/issues/13986.

My understanding of the `copy=False` parameter of estimators is "allow inplace modifications of X".

When avoiding a copy is not possible (X doesn't have the right dtype or memory layout for instance), a copy is still triggered. I believe that X being read-only is a valid reason for still triggering a copy.

My main argument is that the user isn't always in control of the permissions of an input array within the whole pipeline. Especially when joblib parallelism is enabled, which may create read-only memmaps. We've have a bunch of issues because of that, the latest being https://github.com/scikit-learn/scikit-learn/issues/28781. And it's poorly tested because it requires big arrays which we try to avoid in the tests (although joblib 1.13 makes it easy to trigger with small arrays).

I wouldn't make `check_array(copy=False)` always trigger a copy when X is read-only because the semantic of the `copy` param of `check_array` is not the same as the one of estimators. We could introduce a new param in check_array, like `copy_if_readonly` ?
- Estimator has no copy param (i.e.) doesn't intend to do inplace modification:
  `check_array(copy=False, copy_if_readonly=False)`
- Estimator has copy param:
  `check_array(copy=self.copy, copy_if_readonly=True)`

It could also be a third option for copy in check_array: True, False, "if_readonly":
- Estimator has no copy param:
  `check_array(copy=False)`
- Estimator has copy param:
  `check_array(copy=self.copy or "if_readonly")`

COMMENT:
> * How `check_array` deals with read-only memmaps
>   -> We need to make a copy because we can't make a read-only memmap writeable.
> 
> * How `check_array` deals with other read-only array-likes

In the first case, it seems like we should somehow "magically" take care of this for the user. At least I assume only maybe 1% of users know about joblib, memapping, read-only, etc - and even this 1% probably wants things to "just work". So we could do it automatically?

For the second case I don't know enough to know when this happens _and_ a user cares that their arrays are read-only. In the sense that they passed a read only array and care that it is read only and hence not modified.

---

> Thanks for the question @betatim ! At first I was frustrated, feeling that I had already explained it precisely. But in fact it made me think deeper and find more precisely the underlying details of this issue.

Thanks for taking the time! I also get the frustration bit, and don't always manage to take the step back (aka people receive the frustrated me ;) ) but often it is worth re-re-re-explaining because it leads to new ideas/ thoughts. Very much like eating your vegetables, I prefer ice-cream but I hear vegetables are good for you.

Labels: RFC
RFC Trigger a copy when copy=False and X is read-only

Highly related to #14481 and maybe a little bit to https://github.com/scikit-learn/scikit-learn/issues/13986.

My understanding of the `copy=False` parameter of estimators is "allow inplace modifications of X".

When avoiding a copy is not possible (X doesn't have the right dtype or memory layout for instance), a copy is still triggered. I believe that X being read-only is a valid reason for still triggering a copy.

My main argument is that the user isn't always in control of the permissions of an input array within the whole pipeline. Especially when joblib parallelism is enabled, which may create read-only memmaps. We've have a bunch of issues because of that, the latest being https://github.com/scikit-learn/scikit-learn/issues/28781. And it's poorly tested because it requires big arrays which we try to avoid in the tests (although joblib 1.13 makes it easy to trigger with small arrays).

I wouldn't make `check_array(copy=False)` always trigger a copy when X is read-only because the semantic of the `copy` param of `check_array` is not the same as the one of estimators. We could introduce a new param in check_array, like `copy_if_readonly` ?
- Estimator has no copy param (i.e.) doesn't intend to do inplace modification:
  `check_array(copy=False, copy_if_readonly=False)`
- Estimator has copy param:
  `check_array(copy=self.copy, copy_if_readonly=True)`

It could also be a third option for copy in check_array: True, False, "if_readonly":
- Estimator has no copy param:
  `check_array(copy=False)`
- Estimator has copy param:
  `check_array(copy=self.copy or "if_readonly")`

COMMENT:
> In the first case, it seems like we should somehow "magically" take care of this for the user. [...] So we could do it automatically?

yes, what I'm proposing is transparent for the user. It's the responsibility of the estimator to tell `check_array` is it requires a writeable array or not. Then its the responsibility to `check_array` to guarantee to return a writeable array, doing its best to avoid a copy if a copy is not requested.

> For the second case I don't know enough to know when this happens and a user cares that their arrays are read-only. In the sense that they passed a read only array and care that it is read only and hence not modified.

It's true that if a user provides a read-only array but sets `copy=False`, we probably don't want to turn it writeable, but make a copy instead to not risk making a terrible mistake. I need to modify https://github.com/scikit-learn/scikit-learn/pull/29018 accordingly.

Labels: RFC
⚠️ CI failed on Wheel builder (last failure: Apr 11, 2024) ⚠️

**CI is still failing on [Wheel builder](https://github.com/scikit-learn/scikit-learn/actions/runs/8641208872)** (Apr 11, 2024)

COMMENT:
@lesteve I think we broke the string interpolation in the title when merging your last PR :)

Labels: Bug
⚠️ CI failed on Wheel builder (last failure: Apr 11, 2024) ⚠️

**CI is still failing on [Wheel builder](https://github.com/scikit-learn/scikit-learn/actions/runs/8641208872)** (Apr 11, 2024)

COMMENT:
Having said that there seems to be a genuine issue with Wheels. It was passing on the previous scheduled run c1d29ce7fc21690bd69a449c7c3b896e74b76f84 [build log](https://github.com/scikit-learn/scikit-learn/actions/runs/8625652318/job/23642580136) and failing on 24b0e3faed79ac74752141add4586fd6bbb315dd [build log](https://github.com/scikit-learn/scikit-learn/actions/runs/8641208872/job/23690353533)

```
[234/249] Compiling Cython source /project/sklearn/tree/_criterion.pyx
    FAILED: sklearn/tree/_criterion.cpython-310-x86_64-linux-gnu.so.p/sklearn/tree/_criterion.pyx.c
    cython -M --fast-fail -3 '-X language_level=3' '-X boundscheck=False' '-X wraparound=False' '-X initializedcheck=False' '-X nonecheck=False' '-X cdivision=True' '-X profile=False' --include-dir /project/.mesonpy-_kubn7tr /project/sklearn/tree/_criterion.pyx -o sklearn/tree/_criterion.cpython-310-x86_64-linux-gnu.so.p/sklearn/tree/_criterion.pyx.c
  
    Error compiling Cython file:
    ------------------------------------------------------------
    ...
  
    import numpy as np
    cimport numpy as cnp
    cnp.import_array()
  
    from scipy.special.cython_special cimport xlogy
    ^
    ------------------------------------------------------------
  
    /project/sklearn/tree/_criterion.pyx:23:0: 'scipy/special/cython_special.pxd' not found
```

Maybe a scipy-dev new change (since we are building against dev dependencies to prepare Numpy 2)?

Indeed looks like Scipy already knows about it: https://github.com/scipy/scipy/pull/20417#issuecomment-2047809924 and should be solved by https://github.com/scipy/scipy/pull/20422

Labels: Bug
⚠️ CI failed on Wheel builder (last failure: Apr 11, 2024) ⚠️

**CI is still failing on [Wheel builder](https://github.com/scikit-learn/scikit-learn/actions/runs/8641208872)** (Apr 11, 2024)

COMMENT:
## CI is no longer failing! ✅

[Successful run](https://github.com/scikit-learn/scikit-learn/actions/runs/8656646894) on Apr 12, 2024

Labels: Bug
⚠️ CI failed on Wheel builder (last failure: Apr 11, 2024) ⚠️

**CI is still failing on [Wheel builder](https://github.com/scikit-learn/scikit-learn/actions/runs/8641208872)** (Apr 11, 2024)

COMMENT:
scipy/scipy#20422 wasn't merged yet but the last nightly CI was actually successful...

Labels: Bug
⚠️ CI failed on Wheel builder (last failure: Apr 11, 2024) ⚠️

**CI is still failing on [Wheel builder](https://github.com/scikit-learn/scikit-learn/actions/runs/8641208872)** (Apr 11, 2024)

COMMENT:
I believe the actual fix is https://github.com/scipy/scipy/pull/20441 that was merged. Scipy realised that when working in the PR linked above: https://github.com/scipy/scipy/pull/20422#issuecomment-2045980635.

Labels: Bug
Bad rendering of the badge links in the README.rst file on github

E.g. on:

https://github.com/scikit-learn/scikit-learn/blob/main/README.rst

You get something that looks like:

![image](https://github.com/scikit-learn/scikit-learn/assets/89061/f999ca96-877c-4ea6-a669-df9c8714e097)

Notice in particular the first badge for our Azure Pipelines CI that is missing and the trailing underscores that show up everywhere.

However the same `README.rst` contents render well on pypi.org (assuming it has not changed since the last release):

https://pypi.org/project/scikit-learn/

![image](https://github.com/scikit-learn/scikit-learn/assets/89061/a41aa7d2-0b5a-4da2-b52c-d107db9ecce0)

I think it used to be rendered correctly on github a couple of days/weeks ago. Looking at the source of `README.rst` I cannot spot the source of the problem. Has anybody an idea?

COMMENT:
Possibly this is a project setting issue as explained here:

https://stackoverflow.com/questions/58674639/azure-pipelines-status-badge-not-getting-displayed-in-markdown

They had a similar issue and they fixed it by allowing anonymous access to badges.

Labels: Bug, Documentation
Bad rendering of the badge links in the README.rst file on github

E.g. on:

https://github.com/scikit-learn/scikit-learn/blob/main/README.rst

You get something that looks like:

![image](https://github.com/scikit-learn/scikit-learn/assets/89061/f999ca96-877c-4ea6-a669-df9c8714e097)

Notice in particular the first badge for our Azure Pipelines CI that is missing and the trailing underscores that show up everywhere.

However the same `README.rst` contents render well on pypi.org (assuming it has not changed since the last release):

https://pypi.org/project/scikit-learn/

![image](https://github.com/scikit-learn/scikit-learn/assets/89061/a41aa7d2-0b5a-4da2-b52c-d107db9ecce0)

I think it used to be rendered correctly on github a couple of days/weeks ago. Looking at the source of `README.rst` I cannot spot the source of the problem. Has anybody an idea?

COMMENT:
There seems to be a regression in the way github renders rst files:

https://mastodon.social/@webology/112249549385519057

https://github.com/orgs/community/discussions/113792

Labels: Bug, Documentation
Bad rendering of the badge links in the README.rst file on github

E.g. on:

https://github.com/scikit-learn/scikit-learn/blob/main/README.rst

You get something that looks like:

![image](https://github.com/scikit-learn/scikit-learn/assets/89061/f999ca96-877c-4ea6-a669-df9c8714e097)

Notice in particular the first badge for our Azure Pipelines CI that is missing and the trailing underscores that show up everywhere.

However the same `README.rst` contents render well on pypi.org (assuming it has not changed since the last release):

https://pypi.org/project/scikit-learn/

![image](https://github.com/scikit-learn/scikit-learn/assets/89061/a41aa7d2-0b5a-4da2-b52c-d107db9ecce0)

I think it used to be rendered correctly on github a couple of days/weeks ago. Looking at the source of `README.rst` I cannot spot the source of the problem. Has anybody an idea?

COMMENT:
There does not seem to be a permission problem with the Azure badge since I can open it in a private browser window (hence as an an anonymous user):

https://dev.azure.com/scikit-learn/scikit-learn/_apis/build/status/scikit-learn.scikit-learn?branchName=main

Labels: Bug, Documentation
Bad rendering of the badge links in the README.rst file on github

E.g. on:

https://github.com/scikit-learn/scikit-learn/blob/main/README.rst

You get something that looks like:

![image](https://github.com/scikit-learn/scikit-learn/assets/89061/f999ca96-877c-4ea6-a669-df9c8714e097)

Notice in particular the first badge for our Azure Pipelines CI that is missing and the trailing underscores that show up everywhere.

However the same `README.rst` contents render well on pypi.org (assuming it has not changed since the last release):

https://pypi.org/project/scikit-learn/

![image](https://github.com/scikit-learn/scikit-learn/assets/89061/a41aa7d2-0b5a-4da2-b52c-d107db9ecce0)

I think it used to be rendered correctly on github a couple of days/weeks ago. Looking at the source of `README.rst` I cannot spot the source of the problem. Has anybody an idea?

COMMENT:
I found that not only the Azure badge is missing but also all links were not working.
I fired a PR at least for the links.
With this all `_` are not needed anymore, too.

Labels: Bug, Documentation
the documentation says that the min_samples parameter specifies the number of neighbors including the point itself, but does not actually include

### Describe the issue linked to the documentation

The documentation page for the parameters of the DBSCAN mentions that the min_samples parameter : `The number of samples (or total weight) in a neighborhood for a point to be considered as a core point. This includes the point itself.` ([doc](https://scikit-learn.org/stable/modules/generated/sklearn.cluster.DBSCAN.html#sklearn.cluster.DBSCAN.fit))

if you look at the ([source](https://github.com/scikit-learn/scikit-learn/blob/872124551/sklearn/cluster/_dbscan.py#L185)) , you can see that  in neighborhoods 

```
neighborhoods = neighbors_model.radius_neighbors(X, return_distance=False)

if sample_weight is None:
    n_neighbors = np.array([len(neighbors) for neighbors in neighborhoods])
else:
    n_neighbors = np.array([np.sum(sample_weight[neighbors]) for neighbors in neighborhoods])
```
the point itself is not taken into account

```
# A list of all core samples found.
core_samples = np.asarray(n_neighbors >= self.min_samples, dtype=np.uint8)

```

I took min_samples=2 and the points with one neighbor did not become the core

### Suggest a potential alternative/fix

I think need to correct the documentation

COMMENT:
Thanks for opening an issue! I think the documentation is right but I have to admit, I am certainly not a DBSCAN expert.

> I took min_samples=2 and the points with one neighbor did not become the core

Have you tried playing with `eps`? If you can provide a snippet of code showing this issue, this would be great so that a maintainer can have a closer look.

Labels: Documentation, Needs Triage
the documentation says that the min_samples parameter specifies the number of neighbors including the point itself, but does not actually include

### Describe the issue linked to the documentation

The documentation page for the parameters of the DBSCAN mentions that the min_samples parameter : `The number of samples (or total weight) in a neighborhood for a point to be considered as a core point. This includes the point itself.` ([doc](https://scikit-learn.org/stable/modules/generated/sklearn.cluster.DBSCAN.html#sklearn.cluster.DBSCAN.fit))

if you look at the ([source](https://github.com/scikit-learn/scikit-learn/blob/872124551/sklearn/cluster/_dbscan.py#L185)) , you can see that  in neighborhoods 

```
neighborhoods = neighbors_model.radius_neighbors(X, return_distance=False)

if sample_weight is None:
    n_neighbors = np.array([len(neighbors) for neighbors in neighborhoods])
else:
    n_neighbors = np.array([np.sum(sample_weight[neighbors]) for neighbors in neighborhoods])
```
the point itself is not taken into account

```
# A list of all core samples found.
core_samples = np.asarray(n_neighbors >= self.min_samples, dtype=np.uint8)

```

I took min_samples=2 and the points with one neighbor did not become the core

### Suggest a potential alternative/fix

I think need to correct the documentation

COMMENT:
As the [doc](https://scikit-learn.org/stable/modules/generated/sklearn.cluster.DBSCAN.html#sklearn.cluster.DBSCAN) says:
 
> If metric is "precomputed", X is assumed to be a distance matrix and must be square.

Distance matrix, means the matrix has 0 on the diagonal. You matrix is a similarity matrix I am guessing this is why you find points with no neighbors.

I am going to close the issue, since at this point I feel this is more likely to be a scikit-learn usage question rather than a bug in scikit-learn.

Labels: Documentation, Needs Triage
the documentation says that the min_samples parameter specifies the number of neighbors including the point itself, but does not actually include

### Describe the issue linked to the documentation

The documentation page for the parameters of the DBSCAN mentions that the min_samples parameter : `The number of samples (or total weight) in a neighborhood for a point to be considered as a core point. This includes the point itself.` ([doc](https://scikit-learn.org/stable/modules/generated/sklearn.cluster.DBSCAN.html#sklearn.cluster.DBSCAN.fit))

if you look at the ([source](https://github.com/scikit-learn/scikit-learn/blob/872124551/sklearn/cluster/_dbscan.py#L185)) , you can see that  in neighborhoods 

```
neighborhoods = neighbors_model.radius_neighbors(X, return_distance=False)

if sample_weight is None:
    n_neighbors = np.array([len(neighbors) for neighbors in neighborhoods])
else:
    n_neighbors = np.array([np.sum(sample_weight[neighbors]) for neighbors in neighborhoods])
```
the point itself is not taken into account

```
# A list of all core samples found.
core_samples = np.asarray(n_neighbors >= self.min_samples, dtype=np.uint8)

```

I took min_samples=2 and the points with one neighbor did not become the core

### Suggest a potential alternative/fix

I think need to correct the documentation

COMMENT:
@lesteve Thank you very much for the prompt response, now I understand)

Labels: Documentation, Needs Triage
Unexpected behavior of sklearn.feature_selection.mutual_info_regression if copy=False

### Describe the bug

The parameter `copy` of the function `mutual_info_regression` is described as follows https://github.com/scikit-learn/scikit-learn/blob/d1d1596fac19d688a637690134d71fc460f5f0dd/sklearn/feature_selection/_mutual_info.py#L381-L383

I read it as both `X` and `y` should be modified if `copy=False` and `X` has continuous features. However, `y`  is always copied. I think the lines 
https://github.com/scikit-learn/scikit-learn/blob/d1d1596fac19d688a637690134d71fc460f5f0dd/sklearn/feature_selection/_mutual_info.py#L309-L310 should be 
```python
    if not discrete_target:
        y = y.astype(np.float64, copy=copy)
        y = scale(y, with_mean=False, copy=False)
```
Similarly to the the treatment of `X` https://github.com/scikit-learn/scikit-learn/blob/d1d1596fac19d688a637690134d71fc460f5f0dd/sklearn/feature_selection/_mutual_info.py#L295-L299

### Steps/Code to Reproduce

```python
import numpy as np
from sklearn.feature_selection import mutual_info_regression
n_samples_, n_feats = 30, 2
X = np.random.randn(n_samples_, n_feats)
y = np.random.randn(n_samples_, )
y_copy = y.copy()
X_copy = X.copy()
mutual_info_regression(X, y, copy=False)
print(np.allclose(y, y_copy), np.allclose(X, X_copy))
```

### Expected Results

The result should be
```
False, False
```
since both `X` and `y` should be modified in place by the function `mutual_info_regression`. 



### Actual Results

```
True, False
```

### Versions

```shell
System:
    python: 3.11.7 (main, Dec  5 2023, 19:13:35) [GCC 10.2.1 20210110]
executable: /usr/local/bin/python
   machine: Linux-5.15.146.1-microsoft-standard-WSL2-x86_64-with-glibc2.31

Python dependencies:
      sklearn: 1.3.0
          pip: 23.3.1
   setuptools: 69.0.2
        numpy: 1.23.2
        scipy: 1.12.0
       Cython: 3.0.9
       pandas: 2.1.4
   matplotlib: 3.8.2
       joblib: 1.3.2
threadpoolctl: 3.2.0

Built with OpenMP: True

threadpoolctl info:
       user_api: blas
   internal_api: openblas
    num_threads: 12
         prefix: libopenblas
       filepath: /home/user/.local/lib/python3.11/site-packages/numpy.libs/libopenblas64_p-r0-742d56dc.3.20.so
        version: 0.3.20
threading_layer: pthreads
   architecture: Prescott

       user_api: blas
   internal_api: openblas
    num_threads: 12
         prefix: libopenblas
       filepath: /home/user/.local/lib/python3.11/site-packages/scipy.libs/libopenblasp-r0-23e5df77.3.21.dev.so
        version: 0.3.21.dev
threading_layer: pthreads
   architecture: Prescott

       user_api: openmp
   internal_api: openmp
    num_threads: 12
         prefix: libgomp
       filepath: /home/user/.local/lib/python3.11/site-packages/scikit_learn.libs/libgomp-a34b3233.so.1.0.0
        version: None
```

COMMENT:
It would help a lot if you could explain your use case a bit :pray:. In particular how does it affect you that `y` is copied and not `X` when using `copy=False`?

Labels: Documentation, help wanted