Skip to content
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

DOC improve inline comments in SAGA #25100

Merged
merged 6 commits into from
Dec 13, 2022
Merged

Conversation

TomDLT
Copy link
Member

@TomDLT TomDLT commented Dec 2, 2022

Add inline comments to better describe the just-in-time (JIT) update system and the prox operator update in SAGA.

Fixes #24679

@sanjail3
Copy link
Contributor

sanjail3 commented Dec 3, 2022

can i work on this issue

sklearn/linear_model/_sag_fast.pyx.tp Outdated Show resolved Hide resolved
sklearn/linear_model/_sag_fast.pyx.tp Outdated Show resolved Hide resolved
@jjerphan
Copy link
Member

jjerphan commented Dec 5, 2022

@sanjail3: the issue is unfortunately already addressed by this PR.

TomDLT and others added 2 commits December 5, 2022 09:54
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you for bringing your expertise in, @TomDLT.

sklearn/linear_model/_sag_fast.pyx.tp Outdated Show resolved Hide resolved
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan jjerphan added Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels Dec 12, 2022
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 as well. I will commit the following 2 nits and merge.

sklearn/linear_model/_sag_fast.pyx.tp Outdated Show resolved Hide resolved
sklearn/linear_model/_sag_fast.pyx.tp Outdated Show resolved Hide resolved
@ogrisel ogrisel enabled auto-merge (squash) December 13, 2022 10:24
@ogrisel
Copy link
Member

ogrisel commented Dec 13, 2022

Thanks for the improvement @TomDLT. Let me try the auto merge feature on this PR :)

@ogrisel ogrisel merged commit c0eb3d3 into scikit-learn:main Dec 13, 2022
Vincent-Maladiere pushed a commit to Vincent-Maladiere/scikit-learn that referenced this pull request Dec 14, 2022
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Jan 3, 2023
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Jan 23, 2023
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
adrinjalali pushed a commit that referenced this pull request Jan 24, 2023
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
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
Labels
cython Documentation No Changelog Needed Quick Review For PRs that are quick to review 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.

correct SAGA weight update with prox
4 participants