-
-
Notifications
You must be signed in to change notification settings - Fork 26k
MRG Deprecates 'normalize' in LinearRegression (_base.py) #17743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
06de537
d1c9816
233a82e
9369ed3
2e93e06
523d588
a7b7422
15368a7
293682f
6258d1c
2f4d60a
582532a
428f1fa
a93d367
0e89ab9
97c6221
25fe971
0b3e5b5
38b9b5e
7fc22ee
86bb2ef
59e4c87
2e52377
c8d9295
15f2a1e
5a3008b
45b42e3
afc9c01
63f93cc
ca413e1
e546ec1
3886e02
da1ec7d
4e4afe7
e29932d
7f32058
82584f4
6d3ee72
ad7a508
9cceafe
74c30e0
f0eb556
798efcc
abfd95d
3cbb7b0
c8d66b5
d8ee96c
0dbc444
63424b7
117059e
5ae8ece
3aec615
ff6bec1
6d5d588
5170b2c
ac18d63
2c7f8d6
8024a5a
1d8e3de
3ee028e
73c3f70
f0d850c
04dd2b2
ec950dc
509c7c8
1ae7ead
011d751
79a82e9
91fd435
1911725
8c39982
b49d4cc
f0d81fa
0208ee9
a84ac5c
01fbdd1
54e7197
eb913f6
053ceaa
71ba0bf
4a6d7bc
10e7380
4a078f4
a66a14e
9cd4156
787fe67
b0c7bc8
b01c01d
8649118
3d14885
169dae5
d61aad0
511261e
86b1c21
80eb9e5
7aee6ea
64cf6aa
c8f0456
866ced7
505bcd9
f969696
828a3e9
f5e878c
a9403c5
4683f62
1a0dfe1
f1f21f0
620d011
270a946
48c704e
2afefc5
68aa605
c97a6be
0b035a3
48f3b87
57b8819
46d8606
45d9026
4a21c22
ca7f528
8609a9e
84f7be7
81f276d
0653714
80822f7
db71adf
e8d7396
26b729a
ba547a0
4bd9b63
32447a8
5efe0de
e9527e5
6686800
21914d0
69b0080
a149dcc
0036778
7ae55a3
81d34b4
498c430
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ | |
from sklearn.utils._testing import assert_warns_message | ||
from sklearn.utils._testing import ignore_warnings | ||
from sklearn.utils._testing import assert_array_equal | ||
from sklearn.utils._testing import _convert_container | ||
from sklearn.utils._testing import TempMemmap | ||
from sklearn.utils.fixes import parse_version | ||
|
||
|
@@ -301,6 +302,8 @@ def test_lasso_cv_positive_constraint(): | |
assert min(clf_constrained.coef_) >= 0 | ||
|
||
|
||
# FIXME: 'normalize' to be removed in 1.2 | ||
@pytest.mark.filterwarnings("ignore:'normalize' was deprecated") | ||
@pytest.mark.parametrize( | ||
"LinearModel, params", | ||
[(Lasso, {"tol": 1e-16, "alpha": 0.1}), | ||
|
@@ -384,6 +387,60 @@ def test_model_pipeline_same_as_normalize_true(LinearModel, params): | |
assert_allclose(y_pred_normalize, y_pred_standardize) | ||
|
||
|
||
# FIXME: 'normalize' to be removed in 1.2 | ||
@pytest.mark.filterwarnings("ignore:'normalize' was deprecated") | ||
@pytest.mark.parametrize( | ||
"estimator, is_sparse, with_mean", | ||
[(LinearRegression, True, False), | ||
(LinearRegression, False, True), | ||
(LinearRegression, False, False)] | ||
) | ||
def test_linear_model_sample_weights_normalize_in_pipeline( | ||
estimator, is_sparse, with_mean | ||
): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this test is only meant to test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I recall I was proposing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To anticipate this question, I tried to see if this test would pass with the current code for Ridge and Lasso and actually it always fails whether
So the deprecation of their To keep this PR focused, let's just move this test to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes @glemaitre . I answered your comment, but it must have gone lost int the flow of other comments: I will move it to @ogrisel failing for Ridge and Lasso might indeed be a problem as it was supposed to be extended to include them in this test. Why is this the case (for their failing)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @glemaitre Should I create it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We could create one. But it's fine to keep in sklearn/linear_model/tests/test_base.py for now. You can move this test to sklearn/linear_model/tests/test_common.py in a PR that needs to reuse it for another estimator of the |
||
# Test that the results for running linear regression LinearRegression with | ||
# sample_weight set and with normalize set to True gives similar results as | ||
# LinearRegression with no normalize in a pipeline with a StandardScaler | ||
# and set sample_weight. | ||
rng = np.random.RandomState(0) | ||
maikia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
X, y = make_regression(n_samples=20, n_features=5, noise=1e-2, | ||
random_state=rng) | ||
# make sure the data is not centered to make the problem more | ||
# difficult | ||
X += 10 | ||
X_train, X_test, y_train, y_test = train_test_split(X, y, test_size=0.5, | ||
random_state=rng) | ||
if is_sparse: | ||
X_train = sparse.csr_matrix(X_train) | ||
X_test = _convert_container(X_train, 'sparse') | ||
|
||
sample_weight = rng.rand(X_train.shape[0]) | ||
|
||
# linear estimator with explicit sample_weight | ||
reg_with_normalize = estimator(normalize=True) | ||
reg_with_normalize.fit(X_train, y_train, sample_weight=sample_weight) | ||
|
||
# linear estimator in a pipeline | ||
reg_with_scaler = make_pipeline( | ||
StandardScaler(with_mean=with_mean), | ||
estimator(normalize=False) | ||
) | ||
kwargs = {reg_with_scaler.steps[-1][0] + '__sample_weight': | ||
sample_weight} | ||
reg_with_scaler.fit(X_train, y_train, **kwargs) | ||
|
||
y_pred_norm = reg_with_normalize.predict(X_test) | ||
y_pred_pip = reg_with_scaler.predict(X_test) | ||
|
||
assert_allclose( | ||
reg_with_normalize.coef_ * reg_with_scaler[0].scale_, | ||
reg_with_scaler[1].coef_ | ||
) | ||
assert_allclose(y_pred_norm, y_pred_pip) | ||
|
||
|
||
# FIXME: 'normalize' to be removed in 1.2 | ||
@pytest.mark.filterwarnings("ignore:'normalize' was deprecated") | ||
@pytest.mark.parametrize( | ||
"LinearModel, params", | ||
[(Lasso, {"tol": 1e-16, "alpha": 0.1}), | ||
|
Uh oh!
There was an error while loading. Please reload this page.