Fix Ridge sparse + sample_weight + intercept #22899
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
||
|
|
||
| @pytest.mark.parametrize("solver", ["sparse_cg", "sag"]) | ||
| def test_ridge_sample_weights_dense_sparse(solver, global_random_seed): |
There was a problem hiding this comment.
Could this be merged with test_ridge_fit_intercept_sparse (and maybe also test_ridge_fit_intercept_sparse)? (take the best part of both).
It seems kind of duplicate.
There was a problem hiding this comment.
Right, I merged them into a single test. It allowed to find out that lbfgs also had the same issue. I fixed it as well
|
This is great as it will resolve a long existing, severe bug! |
jeremiedbb
commented
Mar 19, 2022
| @@ -1362,33 +1362,41 @@ def test_n_iter(): | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("solver", ["sparse_cg", "lbfgs", "auto"]) | |||
| def test_ridge_fit_intercept_sparse(solver): | |||
| @pytest.mark.parametrize("with_sample_weight", [True, False]) | |||
| def test_ridge_fit_intercept_sparse(solver, with_sample_weight, global_random_seed): | |||
There was a problem hiding this comment.
tested with "all" seeds
|
@agramfort @rth @TomDLT may be interested. |
ogrisel
reviewed
Mar 22, 2022
| For now only sparse_cg and lbfgs can correctly fit an intercept | ||
| with sparse X with default tol and max_iter. | ||
| 'sag' is tested separately in test_ridge_fit_intercept_sparse_sag because it | ||
| requires more iterations and should raise a warning if default max_iter is used. |
There was a problem hiding this comment.
I pushed a new commit to make this true with sample_weight != None. I tested it with "all" global_random_seed.
ogrisel
approved these changes
Mar 22, 2022
glemaitre
pushed a commit
to glemaitre/scikit-learn
that referenced
this pull request
Apr 6, 2022
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
mathijs02
pushed a commit
to mathijs02/scikit-learn
that referenced
this pull request
Dec 27, 2022
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #15438
Same issue as in LinearRegression: in sparse X_offset needs the sample_weight rescaling.
The issue appears with solver = 'sparse-cg' and solver = 'lbfgs'.
I noticed that there's also an issue for 'sag' solver, but I'm not familiar with the code at all. Any help would be greatly appreciated. We can also leave that for a separate PR.ping @lorentzenchr