MRG Sample weight for StandardScaler #18510

maikia · 2020-10-01T20:07:39Z

towards #15601
This PR adds a possibility of defining sample_weight for the StandardScaler (only for the dense X).

Right now #3020 and #17743 are on hold because if we would like to substitute the use of normalize in linear models we need to assure that the suggested way of substitution will work in all cases (ie suggested way is using a Pipeline with a StandardScaler).

However, as pointed out in #18159 this would not work in the case if the linear model is using sample_weight. By adding possibility to add sample_weight to StandardScaler we are making the above substitution valid again.

…into sample_weight_standard_scaler

jnothman · 2020-10-01T21:49:36Z

This should close #15601, right? And it needs to build on #18447

maikia · 2020-10-02T06:24:29Z

Argh. you are right @jnothman . Thanks for pointing this out. I didn't realize there was complete separate conversation on that topic. I will build on #18447 as soon as it's merged

…into sample_weight_standard_scaler

sklearn/preprocessing/_data.py

…into sample_weight_standard_scaler

sklearn/preprocessing/_data.py

…into sample_weight_standard_scaler

sklearn/preprocessing/_data.py

agramfort

I would not have duplicated all tests passing sample weights of ones. To me it's an overkill as the code is covered and tested with a dedicated test. Basically these test which are updated if sample_weight was ignore internally they would pass.

my 2c. Besides LGTM

agramfort · 2020-10-06T15:22:13Z

sklearn/preprocessing/tests/test_data.py

+        y = rng.randn(n_samples)
+        sample_weights_OK = rng.randn(n_samples) ** 2 + 1
+        sample_weights_OK_1 = 1.
+        sample_weights_OK_2 = 2.


I don't get why you test for 1 and 2 as you don't check the consequence on the estimated mean or var. Maybe just do 1?

I would just remove the sample_weights_OK* smoke tests. Valid sample weights are already tested in the other tests. Let's focus this test on invalid sample weights (both wrong shape or wrong length).

I removed most of those sample_weight tests except for the one testing dtype because it caused so much turmoil.
I also had to add np.dot() instead of _safe_accumulator_op() in _incremental_weighted_mean_and_var of extmath.py because it was again causing the dtype not found error.

…mple_weight_standard_scaler

…into sample_weight_standard_scaler

…e not being accepted

…ikia/scikit-learn into sample_weight_standard_scaler

ogrisel · 2020-10-06T16:36:32Z

sklearn/preprocessing/tests/test_data.py

        scaler = StandardScaler()
-        X_scaled = scaler.fit(X).transform(X, copy=True)
+        X_scaled = scaler.fit(X).transform(
+            X, copy=True)


Why this formatting change? I believe the original code was already PEP8 compliant, no?

ogrisel · 2020-10-06T16:36:43Z

sklearn/preprocessing/tests/test_data.py

    scaler = StandardScaler()
-    X_scaled = scaler.fit(X).transform(X, copy=True)
+    X_scaled = scaler.fit(X).transform(
+        X, copy=True)


Same remark here.

ogrisel · 2020-10-06T16:37:09Z

sklearn/preprocessing/tests/test_data.py

@@ -534,12 +617,12 @@ def test_standard_scaler_partial_fit():

    for chunk_size in [1, 2, 50, n, n + 42]:
        # Test mean at the end of the process
-        scaler_batch = StandardScaler(with_std=False).fit(X)
+        scaler_batch = StandardScaler(with_std=False).fit(
+            X)


same remark again.

sklearn/preprocessing/tests/test_data.py

ogrisel · 2020-10-06T16:39:35Z

sklearn/preprocessing/tests/test_data.py

-                 transformer.inverse_transform(
-                     [[np.max(transformer.references_)]]))
+            transformer.inverse_transform(
+                [[np.max(transformer.references_)]]))


Here again those formatting changes are debatable and unrelated to the topic of the PR.

Thanks @ogrisel and @agramfort
This last changes I did because flake8 was failing locally.
However, I did revert them all now according to your suggestions

ogrisel

I would rather avoid introducing unrelated formatted change to limit the risk of introducing potential conflicts in other PRs when merging this PR (plus it would make the history unnecessary complex when using tools such as git blame).

At some point in the future we will standardize on always using an automated formatter such as "black" but in the mean time, let's not change the formatting on unrelated lines.

Other than that, LGTM :)

agramfort

@maikia let's try to revert these formatting changes and then let's merge.

…into sample_weight_standard_scaler

ogrisel · 2020-10-07T08:10:46Z

Thank you very much @maikia!

ogrisel · 2020-10-07T08:41:18Z

For the remaining case of sparse data + missing values + sample_weight, I am not sure which route is best. Extending the existing code that was written for the dense case or start over from independent code as you did earlier in this PR and modify it to add support for missing values with a sparse nan_mask.

maikia · 2020-10-07T08:56:46Z

@ogrisel
since you don't see an advantage over either of them I will reuse what I have wrote previously and try to add nan_mask

maikia added 8 commits September 30, 2020 22:45

initial test sample_weights

001ee5a

added self.mean_ with sample weights

880126e

added the incremental variance for sample_weight

e37c3db

init test

1d9ef4c

test on StandardScaler with sparse input and sample_wegiths

89f1818

flake8

9eeba0f

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

653fb0d

…into sample_weight_standard_scaler

clean up;

892e719

github-actions bot added module:preprocessing module:utils labels Oct 1, 2020

maikia added 7 commits October 2, 2020 11:15

merging master

87cb1e8

adjusting StandardScaler to the new weighted calc

74c2b55

added test on error being raised if sample_weight > 1D

7a5e49a

dirty init with sparse

69d6dfc

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

3ece2fb

…into sample_weight_standard_scaler

making variance work for sparse with sample_weight, cleaning up, tests

c9e7988

clean up

9c04163

agramfort reviewed Oct 3, 2020

View reviewed changes

sklearn/preprocessing/_data.py Outdated Show resolved Hide resolved

sklearn/preprocessing/_data.py Outdated Show resolved Hide resolved

sklearn/preprocessing/_data.py Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

20c5fed

…into sample_weight_standard_scaler

ogrisel reviewed Oct 5, 2020

View reviewed changes

sklearn/preprocessing/_data.py Outdated Show resolved Hide resolved

maikia added 5 commits October 5, 2020 13:38

correct calc for nans with weights

c4476b2

checking if counts_nan is csr or csc for nan calcs

1dea36f

correct update for n_samples_seen with sample_weights

6a72447

checkpoint

d755082

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

498c06d

…into sample_weight_standard_scaler

ogrisel reviewed Oct 5, 2020

View reviewed changes

sklearn/preprocessing/_data.py Outdated Show resolved Hide resolved

ogrisel reviewed Oct 5, 2020

View reviewed changes

sklearn/preprocessing/_data.py Outdated Show resolved Hide resolved

counts_nan for sparse now calculated with multiply and broadcasting

3f0591f

agramfort reviewed Oct 6, 2020

View reviewed changes

ogrisel and others added 7 commits October 6, 2020 17:37

Merge branch 'master' of github.com:scikit-learn/scikit-learn into sa…

b59585f

…mple_weight_standard_scaler

remove some of the tests

d19224b

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

a7bf21b

…into sample_weight_standard_scaler

updated dot product from _safe_accumulator_op to remove error of dtyp…

a40347f

…e not being accepted

removed some of the sample_weight from tests

0187340

Merge branch 'sample_weight_standard_scaler' of https://github.com/ma…

afb7d7a

…ikia/scikit-learn into sample_weight_standard_scaler

clean up

100727c

ogrisel reviewed Oct 6, 2020

View reviewed changes

sklearn/preprocessing/tests/test_data.py Show resolved Hide resolved

ogrisel reviewed Oct 6, 2020

View reviewed changes

ogrisel approved these changes Oct 6, 2020

View reviewed changes

cmarmo added the hacktoberfest-accepted label Oct 6, 2020

agramfort changed the title ~~WIP Sample weight for StandardScaler~~ MRG Sample weight for StandardScaler Oct 6, 2020

agramfort approved these changes Oct 6, 2020

View reviewed changes

maikia added 6 commits October 7, 2020 09:04

clean up

f2a11d3

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

772a0ed

…into sample_weight_standard_scaler

clean up

331449e

clean up

293226d

clean up

94956af

clean up

44a9aab

ogrisel merged commit 64697c6 into scikit-learn:master Oct 7, 2020
7 checks passed

amrcode pushed a commit to amrcode/scikit-learn that referenced this pull request Oct 19, 2020

MRG Sample weight for StandardScaler (scikit-learn#18510)

ed9cb28

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

MRG Sample weight for StandardScaler (scikit-learn#18510)

47ae20e

lorentzenchr mentioned this pull request Feb 10, 2021

Add sample_weight support to StandardScaler (and friends?) #15601

Closed

MRG Sample weight for StandardScaler #18510

MRG Sample weight for StandardScaler #18510

maikia commented Oct 1, 2020 •

edited

jnothman commented Oct 1, 2020

maikia commented Oct 2, 2020

agramfort left a comment

agramfort Oct 6, 2020

ogrisel Oct 6, 2020

maikia Oct 6, 2020

ogrisel Oct 6, 2020

ogrisel Oct 6, 2020

ogrisel Oct 6, 2020

ogrisel Oct 6, 2020

maikia Oct 7, 2020

ogrisel left a comment

agramfort left a comment

ogrisel commented Oct 7, 2020

ogrisel commented Oct 7, 2020

maikia commented Oct 7, 2020

MRG Sample weight for StandardScaler #18510

MRG Sample weight for StandardScaler #18510

Conversation

maikia commented Oct 1, 2020 • edited

jnothman commented Oct 1, 2020

maikia commented Oct 2, 2020

agramfort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

agramfort left a comment

Choose a reason for hiding this comment

ogrisel commented Oct 7, 2020

ogrisel commented Oct 7, 2020

maikia commented Oct 7, 2020

maikia commented Oct 1, 2020 •

edited