Talip7/scikit-learn-issues-multilabel huggingface dataset:
BUG?: PCA output changed in 1.5

Because of changing `u_based_decision=False` in `svd_flip` here:

https://github.com/scikit-learn/scikit-learn/pull/27491/files#diff-b17877cd9b0663deb819cce9f4cc84533c4ca88ca0ebd2380f9c8fc5864acf26R646

The PCA sign flipping differs between sklearn 1.4 and 1.5.0.dev0, see this failing MNE-Python CI:

https://github.com/mne-tools/mne-python/actions/runs/8663512660/job/23757842032?pr=12362#step:17:4581

The short version is that we vendor the 1.4-and-older sklearn code for PCA when `svd_solver="full"` (the only case we use/care about) so we noticed the difference when it changed. I'm not sure it matters much in practice, but it seems bad that a PCA `fit_transform` done in 1.4 is different in 1.5 for the given options. I can replicate locally with this code:
```
import numpy as np
from sklearn.decomposition import PCA
from mne.utils.numerics import _PCA

n_components = 0.9999
n_samples, n_dim = 1000, 10
X = np.random.RandomState(0).randn(n_samples, n_dim)
X[:, -1] = np.mean(X[:, :-1], axis=-1)  # true X dim is ndim - 1
X_orig = X.copy()
pca_skl = PCA(n_components, whiten=False, svd_solver="full")
pca_mne = _PCA(n_components, whiten=False)
X_skl = pca_skl.fit_transform(X)
X_mne = pca_mne.fit_transform(X)
np.testing.assert_allclose(X_mne, X_skl)  # Fails!
```
But if in MNE I change our line to have `svd_flip(..., u_based_decision=False)`:

https://github.com/mne-tools/mne-python/blob/bf74c045d5220682e6e229b95a6e406014c0c73a/mne/utils/numerics.py#L911

It "passes", indicating that this is indeed the difference.

Perhaps this isn't really a bug in the sense that signs are ambiguous in the SVD anyway but a note that these have changed in 1.5 would probably be worthwhile!

*EDIT: Although the comment immediately preceding the changed line is `# flip eigenvectors' sign to enforce deterministic output`, maybe if the idea is for it to be deterministic across sklearn versions as much as possible then this change should be considered a bug? :shrug:*

COMMENT:
I think this is expected: https://scikit-learn.org/dev/whats_new/v1.5.html#changed-models

This is kind of a fix to get a consistent behaviour among all solvers and with `TruncatedSVD`

Labels: Needs Triage
BUG?: PCA output changed in 1.5

Because of changing `u_based_decision=False` in `svd_flip` here:

https://github.com/scikit-learn/scikit-learn/pull/27491/files#diff-b17877cd9b0663deb819cce9f4cc84533c4ca88ca0ebd2380f9c8fc5864acf26R646

The PCA sign flipping differs between sklearn 1.4 and 1.5.0.dev0, see this failing MNE-Python CI:

https://github.com/mne-tools/mne-python/actions/runs/8663512660/job/23757842032?pr=12362#step:17:4581

The short version is that we vendor the 1.4-and-older sklearn code for PCA when `svd_solver="full"` (the only case we use/care about) so we noticed the difference when it changed. I'm not sure it matters much in practice, but it seems bad that a PCA `fit_transform` done in 1.4 is different in 1.5 for the given options. I can replicate locally with this code:
```
import numpy as np
from sklearn.decomposition import PCA
from mne.utils.numerics import _PCA

n_components = 0.9999
n_samples, n_dim = 1000, 10
X = np.random.RandomState(0).randn(n_samples, n_dim)
X[:, -1] = np.mean(X[:, :-1], axis=-1)  # true X dim is ndim - 1
X_orig = X.copy()
pca_skl = PCA(n_components, whiten=False, svd_solver="full")
pca_mne = _PCA(n_components, whiten=False)
X_skl = pca_skl.fit_transform(X)
X_mne = pca_mne.fit_transform(X)
np.testing.assert_allclose(X_mne, X_skl)  # Fails!
```
But if in MNE I change our line to have `svd_flip(..., u_based_decision=False)`:

https://github.com/mne-tools/mne-python/blob/bf74c045d5220682e6e229b95a6e406014c0c73a/mne/utils/numerics.py#L911

It "passes", indicating that this is indeed the difference.

Perhaps this isn't really a bug in the sense that signs are ambiguous in the SVD anyway but a note that these have changed in 1.5 would probably be worthwhile!

*EDIT: Although the comment immediately preceding the changed line is `# flip eigenvectors' sign to enforce deterministic output`, maybe if the idea is for it to be deterministic across sklearn versions as much as possible then this change should be considered a bug? :shrug:*

COMMENT:
Yes, this is an expected change.

Labels: Needs Triage
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:
`if_readonly` makes sense to me, and the estimator would need to know if inplace operations are going to be performed or not.

cc @amueller @thomasjpfan

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:
I'm okay with `check_array(copy=self.copy or "if_readonly")`.

Aside: I alway disliked the semantics of how `copy=False` can sometimes copy. I rather it raise an error if a copy is triggered because of `dtype` or `order`, but it follows `np.array` semantics so I'm okay with keeping the status quo. (I wish there was a `copy="if_required"`)

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:
I see a preference toward ``check_array(copy=True/False/"if_readonly")``. I'd like to try to convince you to have a separate parameter instead of a third copy option.

I see writeable as another property of the resulting array that we want to enforce, exactly like ``order`` and ``dtype``. And both are params of ``check_array``. The semantic of ``copy=False`` already being don't copy unless a conversion is required would be preserved. To be more in line with ``copy`` and `order`, the new param could be ``writeable=True/False``.

If I managed to convince you, great, otherwise I'm fine with ``copy="if_reandonly"`` and I'll move forward with this.

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:
I'm not sure what I think regarding third value vs new argument. It feels like `copy=False` is already confusing enough that adding `copy='if_readonly'` will make it even harder to understand? Which would be a reason for an additional argument. However `check_array(copy=False, copy_if_readonly=True)` is also confusing. At least without reading docs/this discussion I'd have a hard time telling you if this will lead to a copy or not (which of the args wins?).


> 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.

It seems useful to not raise an error in these cases.

After thinking about this a bit more: what is the problem we are trying to solve? 

#13986 makes me think we are trying to avoid bugs (`copy=True` if you don't actually need it sounds like a bug to me). The discussion in #14481 seems to be more about what `copy=False` really means. So yeah, what is the problem we are trying to solve?

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:
> So yeah, what is the problem we are trying to solve?

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.

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:
For my understanding: If we introduce `writable=True/None`, can you link examples in the code where we call `check_array` that will be updated to use `writable=True/None`?

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:
Essentially in the ``_validate_data`` of all estimators that have a ``copy`` parameter because they do inplace operations.

```py
class InplaceEstimator:  # e.g StandardScaler (mostly transformers but not only)
    def __init__(self, copy=True):
        self.copy = copy

    def fit(self, X, y):
        # This estimator performs inplace operations, we need a writeable array
        self._validate_data(X, y, copy=self.copy, writeable=True)


class NotInplaceEstimator:  # e.g. LogisticRegression
    def __init__(self):
        pass

    def fit(self, X, y):
        # This estimator only reads the input data, we don't have to modify the
        # permissions (so nothing to change w.r.t current code)
        self._validate_data(X, y)
```

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:
Thank you for the examples. This makes more sense to me now.

How does the default `copy=False` and `writable=None` handle pandas dataframes after 3.0? As of https://github.com/scikit-learn/scikit-learn/pull/28348, we adjust the NumPy array and make it writable. Does you proposal change this behavior?

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:
> How does the default copy=False and writable=None handle pandas dataframes after 3.0? As of https://github.com/scikit-learn/scikit-learn/pull/28348, we adjust the NumPy array and make it writable. Does you proposal change this behavior?

It does change this behavior. There's no need to change the writeability if we don't intend to perfom inplace operations, so now the array will be made writeable only if ``writeable=True``. Changing the flag like this is not always possible and https://github.com/scikit-learn/scikit-learn/pull/28348 is already the source of new issues like https://github.com/scikit-learn/scikit-learn/issues/28899.

I opened #29018 for a proposed implementation.

Labels: RFC