Skip to content

DEP Deprecate 'normalize' in ridge models #17772

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.

Sign up for GitHub

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

Jump to bottom
Merged
lorentzenchr merged 58 commits into from
Apr 6, 2021
Merged

DEP Deprecate 'normalize' in ridge models #17772

lorentzenchr merged 58 commits into from
Apr 6, 2021

Conversation

@maikia
Copy link
Contributor

@maikia maikia commented Jun 29, 2020

Towards: #3020

It deprecates 'normalize' in _ridge.py:
Ridge
RidgeClassifier
RidgeCV
RidgeClassifierCV

@maikia maikia mentioned this pull request Jul 1, 2020
Merged
@rth rth mentioned this pull request Aug 17, 2020
Closed
Base automatically changed from master to main January 22, 2021 10:52
maikia and others added 21 commits January 22, 2021 15:20
@maikia
merge
486f780
@maikia
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
bc16112 
…nto deprecate_normalize_ridge
@maikia
change deprecate for deprecated
ab113fe
@maikia
use _base._deprecate_normalize for deprecation message
3eaceb3
@maikia
cleanup
7489f7d
@maikia
init test_common.py
85ea259
@maikia
update the test to two parametrize
3559663
@maikia
added ridges to the test
624f203
@maikia
cleanup
f50388a
@maikia
move a test to test_common
716ea57
@maikia
started added tests, not yet running through
e936da9
@maikia
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
652b816 
…nto deprecate_normalize_ridge
@maikia
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
b49e05b 
…nto deprecate_normalize_ridge
@maikia
checkpoint on finding the problem with ridge + sample_weight in a pip…
115794b 
…eline
@maikia
making a new test;
5c8d7ca
@agramfort
bug hunting continues...
1525fe7
@maikia
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
814609e 
…nto deprecate_normalize_ridge
@agramfort
FIX: actually take into account the sample_weights in the X_scale in …
04d09b6 
…linear models... oh well...
@maikia
Merge branch 'deprecate_normalize_ridge' of https://github.com/maikia…
bb8d29f 
…/scikit-learn into deprecate_normalize_ridge
@maikia
merging conflicts
5522977
@maikia
make the tests to pass
7597516
@ogrisel
Copy link
Member

ogrisel commented Mar 12, 2021 •
edited
Loading

I will not merge personally as I've been involved so I'll let a 3rd reviewer click the green button

Maybe @lorentzenchr could have a look? Not that there are still problems with Ridge(normalize=True) and sample_weight but this is investigated separately in #19616.

@lorentzenchr lorentzenchr self-requested a review March 12, 2021 15:52
lorentzenchr
lorentzenchr reviewed Mar 12, 2021
View reviewed changes
Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just some questions. Otherwise this seems solid.

@lorentzenchr
Copy link
Member

lorentzenchr commented Mar 12, 2021

There is a merge conflict. Please merge main in order for the merge button to reappear.

lorentzenchr
lorentzenchr previously approved these changes Mar 27, 2021
View reviewed changes
Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I added a small commit for a bit more detail of the FutureWarning for the elastic net. I hope, it's correct.
@maikia Thank you for this PR and your whole work on deprecating normalize in linear models.

lorentzenchr
lorentzenchr reviewed Mar 27, 2021
View reviewed changes
@lorentzenchr
Copy link
Member

lorentzenchr commented Mar 27, 2021 •
edited
Loading

The test test_enet_ridge_consistency is failing.

@lorentzenchr lorentzenchr dismissed their stale review March 27, 2021 12:52

Failing CI

@agramfort
Copy link
Member

agramfort commented Mar 31, 2021

tried to merge main to see if this helps....

I cannot replicate locally

@lorentzenchr
Copy link
Member

lorentzenchr commented Apr 1, 2021

Test test_enet_ridge_consistency is failing due to a FutureWarning from _deprecate_normalize from Ridge.
Maybe filtering the test as is done elsewhere fixes this problem?

# FIXME: 'normalize' to be removed in 1.2
@pytest.mark.filterwarnings("ignore:'normalize' was deprecated")
def test_enet_ridge_consistency

@ogrisel
Copy link
Member

ogrisel commented Apr 6, 2021

I think you need to pass -Werror to pytest to reproduce the warning to error conversion.

lorentzenchr
lorentzenchr reviewed Apr 6, 2021
View reviewed changes
Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lorentzenchr lorentzenchr changed the title Deprecate 'normalize' in _ridge models DEP Deprecate 'normalize' in ridge models Apr 6, 2021
@lorentzenchr lorentzenchr merged commit 9cfacf1 into scikit-learn:main Apr 6, 2021
@agramfort
Copy link
Member

agramfort commented Apr 6, 2021

thx @maikia @lorentzenchr @ogrisel ! we are getting there !!!

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Apr 19, 2021
@maikia @ogrisel
@agramfort @thomasjpfan
DEP Deprecate 'normalize' in ridge models (scikit-learn#17772)
98e53dd 
Co-authored-by: Olivier Grisel <[email protected]>
Co-authored-by: Alexandre Gramfort <[email protected]>
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
@lorentzenchr lorentzenchr mentioned this pull request Jun 18, 2021
Closed
sebp added a commit to sebp/scikit-survival that referenced this pull request Dec 28, 2021
@sebp
Fix deprecation warning of normalize parameter of IPCRidge
fcaf5a5 
Deprecated in sklearn:
scikit-learn/scikit-learn#17772
sebp added a commit to sebp/scikit-survival that referenced this pull request Dec 30, 2021
@sebp
Fix deprecation warning of normalize parameter of IPCRidge
1264f3b 
Deprecated in sklearn:
scikit-learn/scikit-learn#17772
sebp added a commit to sebp/scikit-survival that referenced this pull request Dec 30, 2021
@sebp
Fix deprecation warning of normalize parameter of IPCRidge
29c3f53 
Deprecated in sklearn:
scikit-learn/scikit-learn#17772
@thomasjpfan thomasjpfan mentioned this pull request Apr 13, 2022
Closed
@eddiebergman eddiebergman mentioned this pull request Nov 15, 2022
Closed
54 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogrisel ogrisel ogrisel approved these changes

@lorentzenchr lorentzenchr lorentzenchr left review comments

@agramfort agramfort agramfort approved these changes

Assignees

No one assigned

Labels

module:linear_model

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maikia @ogrisel @lorentzenchr @agramfort