Skip to content

ENH/FIX stopping criterion for coordinate descent gap <= tol #31906

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
merged 8 commits into from
Aug 13, 2025

Conversation

lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented Aug 9, 2025

Reference Issues/PRs

Noted already in #20144.

What does this implement/fix? Explain your changes.

The stopping criterion in the main branch reads:
gap < tol, but a better version is gap <= tol.

Any other comments?

Reduce (or explicitly suppress) many Convergence warnings in test suite, see #4780.

Copy link

github-actions bot commented Aug 9, 2025

✔️ Linting Passed

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

Generated for commit: 8426f12. Link to the linter CI: here

@lorentzenchr lorentzenchr changed the title ENH/FIX stopping criterion for coordinate descent tol <= gap ENH/FIX stopping criterion for coordinate descent tol <= gap Aug 9, 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 @lorentzenchr

@OmarManzoor OmarManzoor added the Waiting for Second Reviewer First reviewer is done, need a second one! label Aug 11, 2025
Copy link
Member

@virchan virchan 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 PR, @lorentzenchr!

Just a few minor issues with the rendered docs:

Screenshot 2025-08-11 212958

Otherwise, LGTM!

We also have a few other PRs updating _coordinate_descent.py (e.g., #31882, #31909), so at some point we might need to sort out the merge order.

@lorentzenchr lorentzenchr changed the title ENH/FIX stopping criterion for coordinate descent tol <= gap ENH/FIX stopping criterion for coordinate descent gap <= tol Aug 12, 2025
@lorentzenchr
Copy link
Member Author

I had missed the parameter docstring of tol, fixed in fea0c15.

@lorentzenchr
Copy link
Member Author

We also have a few other PRs updating _coordinate_descent.py, so at some point we might need to sort out the merge order.

This one first, please.

@lorentzenchr
Copy link
Member Author

Technically, this PR has the approval of two core contributors. @OmarManzoor wanna merge?

lorentzenchr and others added 2 commits August 13, 2025 06:49
@OmarManzoor
Copy link
Contributor

LGTM. Let's enable auto-merge

@OmarManzoor OmarManzoor enabled auto-merge (squash) August 13, 2025 05:39
@OmarManzoor OmarManzoor removed the Waiting for Second Reviewer First reviewer is done, need a second one! label Aug 13, 2025
@OmarManzoor OmarManzoor merged commit 33a733e into scikit-learn:main Aug 13, 2025
37 checks passed
@lorentzenchr lorentzenchr deleted the cd_tol branch August 13, 2025 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants