Repository: scikit-learn/scikit-learn
Pull request: Fix sample_weight overwriting bug
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->

Fixes #18347

#### What does this implement/fix? Explain your changes.

Add `_check_sample_weight` to `copy=False/True`

#### Any other comments?
 
#18480 is in progress, but I created this PR because the worker has been deleted

<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
Use of class_weight dictionary in LogisticRegression modifies sample_weight object

#### Describe the bug
When a dictionary is supplied to keyword argument `class_weight` with unequal weights and `sample_weight` is specified, the sample weight object is modified.

#### Steps/Code to Reproduce
```python
from sklearn.datasets import load_iris
from sklearn.linear_model import LogisticRegression
from sklearn.model_selection import train_test_split
import numpy as np

X, y = load_iris(return_X_y=True)
np.random.seed(1234)
W = np.random.random(len(X)) * 10.0

print('Sum of weight before: {}'.format(W.sum()))

# fit model
clf = LogisticRegression(random_state=0,
                         class_weight={0: 1.0, 1: 10.0, 2: 1.0},
                         max_iter=200)
clf.fit(X, y, sample_weight=W)

print('Sum of weight after: {}'.format(W.sum()))
```

Produces results:
```
Sum of weight before: 761.8436984163643
Sum of weight after: 3075.667075252882
```

#### Expected Results
Weight object (`W`) should be the unaltered after `LogisticRegression.fit` call.

The expected result can be achieved by deep copying the weight object:

```python
# fit model
import copy
clf = LogisticRegression(random_state=0,
                         class_weight={0: 1.0, 1: 10.0, 2: 1.0},
                         max_iter=200)
clf.fit(X, y, sample_weight=copy.deepcopy(W))
```

```
Sum of weight before: 761.8436984163643
Sum of weight after: 761.8436984163643
```

#### Actual Results
Weight object (`W`) was modified (sum of weights before was 761.8, after 3075.7).

#### Versions
```
System:
    python: 3.8.3 (default, May 19 2020, 06:50:17) [MSC v.1916 64 bit (AMD64)]
executable: C:\Users\kusan\work\bayes_inj_risk\.venv\Scripts\python.exe
   machine: Windows-10-10.0.19041-SP0

Python dependencies:
          pip: 20.2.2
   setuptools: 49.6.0
      sklearn: 0.23.2
        numpy: 1.19.1
        scipy: 1.5.2
       Cython: None
       pandas: 1.1.1
   matplotlib: 3.3.1
       joblib: 0.16.0
threadpoolctl: 2.1.0

Built with OpenMP: True
```

Hints from issue discussion:
Whoops. We should have a `check_array` that should make a copy if we are going to modify the weights.
@KrisKusano Thanks for the report

Commit message: Change the default copy param from False to True

Patch to review:
diff --git a/sklearn/linear_model/tests/test_logistic.py b/sklearn/linear_model/tests/test_logistic.py
index 480472504a92b..0c91ddce290fb 100644
--- a/sklearn/linear_model/tests/test_logistic.py
+++ b/sklearn/linear_model/tests/test_logistic.py
@@ -1869,3 +1869,28 @@ def test_multinomial_identifiability_on_iris(fit_intercept):
     assert_allclose(clf.coef_.sum(axis=0), 0, atol=1e-10)
     if fit_intercept:
         clf.intercept_.sum(axis=0) == pytest.approx(0, abs=1e-15)
+
+
+@pytest.mark.parametrize("multi_class", {'ovr', 'multinomial', 'auto'})
+@pytest.mark.parametrize("class_weight", [
+    {0: 1.0, 1: 10.0, 2: 1.0}, 'balanced'
+])
+def test_sample_weight_not_modified(multi_class, class_weight):
+    X, y = load_iris(return_X_y=True)
+    n_features = len(X)
+    W = np.ones(n_features)
+    W[:n_features // 2] = 2
+
+    expected = W.sum()
+
+    clf = LogisticRegression(random_state=0,
+                             class_weight=class_weight,
+                             max_iter=200,
+                             multi_class=multi_class)
+    clf.fit(X, y, sample_weight=W)
+    actual = W.sum()
+    msg = (
+        f'Sum of weight before ({expected}) should be the same as'
+        f'sum if weight after ({actual})'
+    )
+    assert_allclose(expected, actual, err_msg=msg)
diff --git a/sklearn/utils/validation.py b/sklearn/utils/validation.py
index 902a9f4ddf426..f5be01ddd94eb 100644
--- a/sklearn/utils/validation.py
+++ b/sklearn/utils/validation.py
@@ -1273,7 +1273,7 @@ def _check_psd_eigenvalues(lambdas, enable_warnings=False):
     return lambdas
 
 
-def _check_sample_weight(sample_weight, X, dtype=None):
+def _check_sample_weight(sample_weight, X, dtype=None, copy=True):
     """Validate sample weights.
 
     Note that passing sample_weight=None will output an array of ones.
@@ -1296,6 +1296,9 @@ def _check_sample_weight(sample_weight, X, dtype=None):
        is be allocated.  If `dtype` is not one of `float32`, `float64`,
        `None`, the output will be of dtype `float64`.
 
+    copy : bool, default=False
+        If True, a copy of sample_weight will be created.
+
     Returns
     -------
     sample_weight : ndarray of shape (n_samples,)
@@ -1315,7 +1318,7 @@ def _check_sample_weight(sample_weight, X, dtype=None):
             dtype = [np.float64, np.float32]
         sample_weight = check_array(
             sample_weight, accept_sparse=False, ensure_2d=False, dtype=dtype,
-            order="C"
+            order="C", copy=copy
         )
         if sample_weight.ndim != 1:
             raise ValueError("Sample weights must be 1D array or scalar")


Code review comments:
diff_hunk: @@ -1869,3 +1869,28 @@ def test_multinomial_identifiability_on_iris(fit_intercept):
     assert_allclose(clf.coef_.sum(axis=0), 0, atol=1e-10)
     if fit_intercept:
         clf.intercept_.sum(axis=0) == pytest.approx(0, abs=1e-15)
+
+
+@pytest.mark.parametrize("multi_class", {'ovr', 'multinomial', 'auto'})
+@pytest.mark.parametrize("class_weight", [
+    {0: 1.0, 1: 10.0, 2: 1.0}, 'balanced'
+])
+def test_sample_weight_not_modified(multi_class, class_weight):
+    X, y = load_iris(return_X_y=True)
+    n_features = len(X)
+    W = np.ones(n_features)
+    W[:n_features // 2] = 2
+
+    expected = W.sum()
+
+    clf = LogisticRegression(random_state=0,
+                             class_weight=class_weight,
+                             max_iter=200,
+                             multi_class=multi_class)
+    clf.fit(X, y, sample_weight=W)
+    actual = W.sum()
+    msg = (
+        f'Sum of weight before ({expected}) should be the same as'
+        f'sum if weight after ({actual})'
+    )
path: sklearn/linear_model/tests/test_logistic.py
text: @user1:
This is not necessary. When assert_allclose fails there's already an explicit error message with the 2 values printed.

diff_hunk: @@ -1869,3 +1869,28 @@ def test_multinomial_identifiability_on_iris(fit_intercept):
     assert_allclose(clf.coef_.sum(axis=0), 0, atol=1e-10)
     if fit_intercept:
         clf.intercept_.sum(axis=0) == pytest.approx(0, abs=1e-15)
+
+
+@pytest.mark.parametrize("multi_class", {'ovr', 'multinomial', 'auto'})
+@pytest.mark.parametrize("class_weight", [
+    {0: 1.0, 1: 10.0, 2: 1.0}, 'balanced'
+])
+def test_sample_weight_not_modified(multi_class, class_weight):
+    X, y = load_iris(return_X_y=True)
+    n_features = len(X)
+    W = np.ones(n_features)
+    W[:n_features // 2] = 2
+
+    expected = W.sum()
path: sklearn/linear_model/tests/test_logistic.py
text: @user1:
I'd prefer ``expected = W.copy()`` so that we are sure that each element hasn't been modified.