-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Add tol
to LinearRegression
#30521
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
Add tol
to LinearRegression
#30521
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
Please add an entry such as:
LinearRegression: {"check_sample_weight_equivalence_on_dense_data": dict(tol=1e-12)},
in the PER_ESTIMATOR_CHECK_PARAMS
dict defined in sklearn/utils/_test_common/instance_generator.py
and then remove the matching entry from the PER_ESTIMATOR_XFAIL_CHECKS
dict in the same file. This should help check that we can use this to actually fix #30131.
You can run the common tests locally using:
pytest -vlxk "check_sample_weight_equivalence_on_sparse_data and LinearRegression" sklearn/tests/test_common.py
Actually, we also need to remove the xfail marker in the parametrization of This should involve setting a low enough value for |
Also, there are failing tests on the CI because the EDIT: more precisely: "tol": [Interval(Real, 0, None, closed="left")] as defined in the |
We will also need a dedicated changelog entry under |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
The merge of #30535 into the You are welcome to use the "Resolve conflicts" button to use the online GitHub editor to do so, but don't forget to pull the result into your local branch before doing any subsequent commits locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks very much @SuccessMoses!
doc/whats_new/upcoming_changes/sklearn.linear_model/30521.fix.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @SuccessMoses
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Fixes #30131. I added
tol
toLinearRegression.__init__
this value is passed to thescipy.sparse.linalg.lsqr
.Any other comments?
No.