Skip to content

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

Merged

Conversation

SuccessMoses
Copy link
Contributor

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Fixes #30131. I added tol to LinearRegression.__init__ this value is passed to the scipy.sparse.linalg.lsqr.

Any other comments?

No.

Copy link

github-actions bot commented Dec 20, 2024

✔️ Linting Passed

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

Generated for commit: 2fc18a1. Link to the linter CI: here

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.

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

@ogrisel
Copy link
Member

ogrisel commented Dec 23, 2024

Actually, we also need to remove the xfail marker in the parametrization of test_linear_regression_sample_weight_consitency (and fix the typo in that test name to test_linear_regression_sample_weight_consistency while we are at it).

This should involve setting a low enough value for tol in the body of that test function.

@ogrisel
Copy link
Member

ogrisel commented Dec 23, 2024

Also, there are failing tests on the CI because the _parameter_constraints attribute of the LinearRegression class has not been updated to define the constraints of that new parameter. It should be a real value in the [0, None) range (None meaning undefined upper bound, that is +inf in practice).

EDIT: more precisely:

"tol": [Interval(Real, 0, None, closed="left")]

as defined in the _parameter_constraints attribute of the _BaseRidge class.

@ogrisel
Copy link
Member

ogrisel commented Dec 23, 2024

We will also need a dedicated changelog entry under doc/whats_new/upcoming_changes/sklearn.linear_model/.

@ogrisel
Copy link
Member

ogrisel commented Dec 30, 2024

The merge of #30535 into the main branch has caused two conflicts in sklearn/utils/_test_common/instance_generator.py. This is causing some of the CI to fail. Please merge the main branch into your branch, resolve the conflicts, and push the resulting merge commit with the resolved conflicts to your PR to re-run the CI on the results.

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.

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, thanks very much @SuccessMoses!

@ogrisel ogrisel added the Waiting for Second Reviewer First reviewer is done, need a second one! label Jan 12, 2025
Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @SuccessMoses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:linear_model Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LinearRegression on sparse matrices is not sample weight consistent
3 participants