MRG Deprecates 'normalize' in LinearRegression (_base.py) by maikia · Pull Request #17743 · scikit-learn/scikit-learn

maikia · 2020-06-26T12:25:10Z

Towards: #3020

It deprecates 'normalize' in _base.py (LinearRegression)

…into depreciate_normalize_base

sklearn/linear_model/_base.py

sklearn/linear_model/tests/test_base.py

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

…/scikit-learn into depreciate_normalize_base

maikia · 2020-06-26T16:31:42Z

@rth @agramfort @glemaitre
what do you think?

(problem with the docs should hopefully be fixed soon: #17745)

sklearn/linear_model/_base.py

…into depreciate_normalize_base

agramfort

you will also need an entry in what's new do document the deprecation

besides LTGM provided CIs are happy (doc build included)

sklearn/linear_model/_base.py

…ll warnings

…into depreciate_normalize_base

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

…into depreciate_normalize_base

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

…er test the impact of with_mean

ogrisel

I pushed a new commit to change the new test to see the impact of the with_mean parameter of the StandardScaler for dense inputs.

So apparently, both with_mean=True and with_mean=False work for dense data on LinearRegression. I assume the mean feature value is moved to the intercept and therefore scaling with or without mean does change the equivalence asserted in the test.

However I am not sure about how regularization will impact this if we are to write a similar test for Ridge and Lasso for instance.

ogrisel · 2021-01-21T08:41:45Z

The test failure is unrelated and reported in a dedicated issue: #19224.

ogrisel

In light of the updated test, I think it's fine to keep an explicit with_mean=False in the deprecation message.

LGTM for merge once the following comment is addressed:

ogrisel · 2021-01-21T08:43:19Z

sklearn/linear_model/tests/test_coordinate_descent.py

+)
+def test_linear_model_sample_weights_normalize_in_pipeline(
+        estimator, is_sparse, with_mean
+):


If this test is only meant to test LinearRegression it should be moved to sklearn/linear_model/tests/test_base.py. If it's meant to be extended to Ridge, Lasso... maybe it should be move to a new file, e.g. sklearn/linear_model/tests/test_linear_model.py

If I recall I was proposing sklearn/linear_model/tests/test_common.py that is the usual way that we structure common tests for a module.

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 with_mean is True or False on dense data and it also fails with with_mean=False on sparse data:

sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_model_sample_weights_normalize_in_pipeline[LinearRegression-True-False] PASSED [ 12%] sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_model_sample_weights_normalize_in_pipeline[LinearRegression-False-True] PASSED [ 25%] sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_model_sample_weights_normalize_in_pipeline[LinearRegression-False-False] PASSED [ 37%] sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_model_sample_weights_normalize_in_pipeline[Ridge-True-False] FAILED [ 50%] sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_model_sample_weights_normalize_in_pipeline[Ridge-False-True] FAILED [ 62%] sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_model_sample_weights_normalize_in_pipeline[Ridge-False-False] FAILED [ 75%] sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_model_sample_weights_normalize_in_pipeline[Lasso-False-True] FAILED [ 87%] sklearn/linear_model/tests/test_coordinate_descent.py::test_linear_model_sample_weights_normalize_in_pipeline[Lasso-False-False] FAILED [100%]

So the deprecation of their normalize option should not be implemented with the same message I believe.

To keep this PR focused, let's just move this test to sklearn/linear_model/tests/test_base.py for now.

Yes @glemaitre . I answered your comment, but it must have gone lost int the flow of other comments:
#17743 (comment)

I will move it to test_base.py.

@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)?

@glemaitre
There is no file: sklearn/linear_model/tests/test_common.py
(there is: sklearn/tests/test_common.py, hence my previous question above).

Should I create it?

There is no file: sklearn/linear_model/tests/test_common.py

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 sklearn.linear_model module.

…into depreciate_normalize_base

ogrisel · 2021-01-22T13:58:07Z

Merged. The rest of the discussion can be handled in PRs related to Ridge and Lasso.

agramfort · 2021-01-22T17:21:35Z

congrats and thanks @maikia !

maikia added 5 commits June 26, 2020 13:52

first normalize changes

06de537

exchanged setting self.normalize by _normalize

d1c9816

updated the warning

233a82e

clean up

9369ed3

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

2e93e06

…into depreciate_normalize_base

github-actions bot added the module:linear_model label Jun 26, 2020

maikia added 2 commits June 26, 2020 15:23

added test if warnings do show up

523d588

clean up

a7b7422

glemaitre reviewed Jun 26, 2020

View reviewed changes

maikia and others added 11 commits June 26, 2020 16:11

change of the warning msg

15368a7

clean up

293682f

updated warning msg

6258d1c

updated warning msg

2f4d60a

removed ignore warning from the test

582532a

Update sklearn/linear_model/tests/test_base.py

428f1fa

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

cleaning up the test

a93d367

Update sklearn/linear_model/tests/test_base.py

0e89ab9

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

cleaning up the test

97c6221

Merge branch 'depreciate_normalize_base' of https://github.com/maikia…

25fe971

…/scikit-learn into depreciate_normalize_base

updated tests in test_coordinate_descent

0b3e5b5

agramfort reviewed Jun 26, 2020

View reviewed changes

sklearn/linear_model/_base.py Outdated Show resolved Hide resolved

thomasjpfan mentioned this pull request Jun 27, 2020

FIX Extract estimator objects before aggregating dict of scores #17745

Merged

maikia added 2 commits June 29, 2020 10:00

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

38b9b5e

…into depreciate_normalize_base

removed with_mean=False from standardScaler

7fc22ee

agramfort reviewed Jun 29, 2020

View reviewed changes

sklearn/linear_model/_base.py Outdated Show resolved Hide resolved

added private function _deprecate_normalize(normalize, default) to ca…

86bb2ef

…ll warnings

maikia changed the title ~~WIP: Deprecates 'normalize' in LinearRegression (_base.py)~~ Deprecates 'normalize' in LinearRegression (_base.py) Jun 29, 2020

glemaitre mentioned this pull request Jun 29, 2020

Sphinx issue with autosummary #17771

Closed

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

59e4c87

…into depreciate_normalize_base

maikia and others added 7 commits January 18, 2021 14:06

Update sklearn/linear_model/_base.py

6686800

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

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

21914d0

…into depreciate_normalize_base

update a doc

69b0080

add the doc to the test

a149dcc

Update sklearn/linear_model/_base.py

0036778

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update sklearn/linear_model/_base.py

7ae55a3

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Extend test_linear_model_sample_weights_normalize_in_pipeline to bett…

81d34b4

…er test the impact of with_mean

ogrisel reviewed Jan 20, 2021

View reviewed changes

ogrisel mentioned this pull request Jan 21, 2021

check_decision_proba_consistency fails with LinearDiscriminantAnalysis #19224

Open

ogrisel approved these changes Jan 21, 2021

View reviewed changes

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

498c430

…into depreciate_normalize_base

Base automatically changed from master to main January 22, 2021 10:52

ogrisel merged commit 306826f into scikit-learn:main Jan 22, 2021

ogrisel modified the milestones: 1.0, 0.24.2, 0.24.1 Feb 2, 2021

maikia mentioned this pull request Feb 10, 2021

MRG fix Normalize for linear models when used with sample_weight #19426

Merged

lorentzenchr mentioned this pull request Mar 9, 2021

[MRG] Add quantile regression #9978

Merged

glemaitre mentioned this pull request Apr 22, 2021

Release 0.24.2 #19954

Merged

12 tasks

lorentzenchr mentioned this pull request Jun 18, 2021

Normalize only applies if fit_intercept=True #3020

Closed

thomasjpfan mentioned this pull request Apr 13, 2022

"normalize" parameter in sklearn.linear_model should be "standardize" #16445

Closed

mmccarty mentioned this pull request Jul 1, 2022

[BUG] 'normalize' in LinearRegression deprecated in scikit-learn 1.0 rapidsai/cuml#4795

Closed

eddiebergman mentioned this pull request Nov 15, 2022

Update scikit learn 1.2 automl/auto-sklearn#1611

Closed

54 tasks

dvasya mentioned this pull request Dec 12, 2022

Linear regressions not working with sklearn 1.2 due to normalize argument removal JuliaAI/MLJScikitLearnInterface.jl#45

Closed

Uh oh!

Conversation

maikia commented Jun 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maikia commented Jun 26, 2020

Uh oh!

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Jan 21, 2021

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

glemaitre Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

ogrisel Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

maikia Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

maikia Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Jan 22, 2021

Uh oh!

agramfort commented Jan 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maikia Jan 21, 2021 •

edited

Loading

ogrisel Jan 22, 2021 •

edited

Loading