Skip to content

MNT Change LinearRegression default tol to not break behavior #30688

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
merged 3 commits into from
Jan 23, 2025
Merged

MNT Change LinearRegression default tol to not break behavior #30688

merged 3 commits into from
Jan 23, 2025

Conversation

jeremiedbb
Copy link
Member

Fixes #30684

Now that LinearRegression has a tol parameter we need to set it to a low value if we want to compare with the exact solution.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 21, 2025 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 3b0c267. Link to the linter CI: here

@jeremiedbb jeremiedbb mentioned this pull request Jan 21, 2025
Closed
ogrisel
ogrisel approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM!

@ogrisel
Copy link
Member

ogrisel commented Jan 21, 2025 •
edited
Loading

No need to backport to 1.6.X since #30521 was merged after the 1.6.0 release and the docstring mentions that the new tol parameter is intended to be released as part of the future 1.7.

@jeremiedbb
[all random seeds]
302bbd0 
test_linear_regression_sample_weights
@lesteve
Copy link
Member

lesteve commented Jan 21, 2025

The change to the test seems fine, but at the same time this means that there is a behaviour change in #30521, right?

Maybe this should be mentioned in the changelog?

@jeremiedbb
Copy link
Member Author

The change to the test seems fine, but at the same time this means that there is a behaviour change in #30521, right?

Maybe this should be mentioned in the changelog?

Alternatively we can set the default tol to 1e-6, which is the default for scipy lsqr (atol=1e-06, btol=1e-06), such that we don't change the behavior.

@jeremiedbb
Copy link
Member Author

jeremiedbb commented Jan 22, 2025 •
edited by lesteve
Loading

Reading more the issues and PRs leading to adding the tol parameter I didn't see any specific motivation to set it to 1e-4 by default. My guess is that it's the default tol of Ridge. The snippet in the original issue requires a tol as low as at least 1e-12 to pass, which is reflected in the common test.

So I think that we can change the default to 1e-6 now, which is harmless since it hasn't been released yet, to avoid the behavior change. We can consider changing the default later to match Ridge if we really want to but it doesn't feel necessary.

@jeremiedbb jeremiedbb changed the title TST Fix test for LinearRegression with sample_weights MNT Change LinearRegression default tol to not break behavior Jan 22, 2025
@lesteve
Copy link
Member

lesteve commented Jan 22, 2025 •
edited
Loading

Thanks for having a closer look!

I don't have a strong opinion on what is preferable:

  • tol=1e-6: more conservative by avoiding the behaviour change
  • tol=1e-4: consistency with default Ridge tol (if there is not a stronger argument) but introduces a behaviour change. Some may argue that LinearRegression should not be used for anything serious (you should always use Ridge with a small penalty), so maybe breaking backward compatibility is OKish

@jeremiedbb
Copy link
Member Author

@ogrisel, @glemaitre what do you think ?

@ogrisel
Copy link
Member

ogrisel commented Jan 23, 2025

Let's do the backward compat for now. My long term plan would be to align LinearRegression better with ridge but this will involve more work.

lesteve
lesteve reviewed Jan 23, 2025
View reviewed changes
sklearn/linear_model/tests/test_base.py
@@ -72,7 +72,7 @@ def test_linear_regression_sample_weights(
sample_weight = 1.0 + rng.uniform(size=n_samples)

# LinearRegression with explicit sample_weight
reg = LinearRegression(fit_intercept=fit_intercept)
reg = LinearRegression(fit_intercept=fit_intercept, tol=1e-16)
Copy link
Member

Choose a reason for hiding this comment

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

So I guess this is not needed if we go for backward-compat i.e. tol=1e-6 by default?

Suggested change
reg = LinearRegression(fit_intercept=fit_intercept, tol=1e-16)
reg = LinearRegression(fit_intercept=fit_intercept)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would keep it anyway because it's kind of "lucky" that it works with a higher tol. The reason tol was added was because it made a sample weight test fail and this test is also checking sample weights. Those tests require good convergence.

Copy link
Member

Choose a reason for hiding this comment

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

OK fair enough!

@lesteve
Copy link
Member

lesteve commented Jan 23, 2025

Let's merge this one, thanks!

@lesteve lesteve merged commit 73db8f1 into scikit-learn:main Jan 23, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesteve lesteve lesteve left review comments

@ogrisel ogrisel ogrisel approved these changes

Assignees
No one assigned
Labels
module:linear_model No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

⚠️ CI failed on Linux_Runs.pylatest_conda_forge_mkl (last failure: Jan 21, 2025) ⚠️
3 participants
@jeremiedbb @ogrisel @lesteve