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

[FEA] Support L1 regularization and ElasticNet in MNMG Dask LogisticRegression #5587

Merged
merged 5 commits into from
Oct 4, 2023

Conversation

lijinf2
Copy link
Contributor

@lijinf2 lijinf2 commented Sep 20, 2023

This PR depends on PR5565.

@github-actions github-actions bot added Cython / Python Cython or Python issue CUDA/C++ labels Sep 20, 2023
@lijinf2 lijinf2 added breaking Breaking change improvement Improvement / enhancement to an existing function labels Sep 20, 2023
@lijinf2 lijinf2 marked this pull request as ready for review September 22, 2023 17:13
@lijinf2 lijinf2 requested review from a team as code owners September 22, 2023 17:13
@dantegd dantegd added the 3 - Ready for Review Ready for review by team label Sep 25, 2023
@csadorf
Copy link
Contributor

csadorf commented Sep 26, 2023

@dantegd Ok to change the base to #5565 to simplify review?

@copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 27, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@csadorf csadorf self-assigned this Sep 29, 2023
@dantegd dantegd added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Oct 2, 2023
@lijinf2 lijinf2 added 3 - Ready for Review Ready for review by team and removed 4 - Waiting on Author Waiting for author to respond to review labels Oct 3, 2023
@lijinf2
Copy link
Contributor Author

lijinf2 commented Oct 3, 2023

@csadorf Thanks for the review! Just submitted the revised PR. Please help take a look.

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

@lijinf2 I'm sorry for being late to this review but LBFGS expects differentiable objectives and the lasso objective, while being convex, is not differentiable everywhere.

The owl-qn method is an extension of lbfgs that supports lasso. The single-gpu lasso-logistic uses owl-qn and we should be doing the same for the distributed version as well.

@lijinf2
Copy link
Contributor Author

lijinf2 commented Oct 3, 2023

@lijinf2 I'm sorry for being late to this review but LBFGS expects differentiable objectives and the lasso objective, while being convex, is not differentiable everywhere.

The owl-qn method is an extension of lbfgs that supports lasso. The single-gpu lasso-logistic uses owl-qn and we should be doing the same for the distributed version as well.

Thanks for helping look at the PR! The distributed version has been modified to reuse single-gpu qn_minimize instead of single-gpu min_lbfgs in a recent PR #5558. As a result, the qn_minimize will automatically choose min_owlqn when l1 penalty is non zero, and min_lbfgs when l1 penalty is zero.

In my local pytest verbose logs, "qn_solvers.cuh:323 Running OWL-QN" was printed out in all test cases of test_l1 and test_elasticnet, while "qn_solvers.cuh:180 Running L-BFGS" was printed out in all test cases of test_lbfgs (penalty is l2) and test_noreg (penalty is none). So it seems the distributed version has been doing the same as single-gpu.

Let me know if you meant to revise the code or test cases!

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

In my local pytest verbose logs, "qn_solvers.cuh:323 Running OWL-QN" was printed out in all test cases of test_l1

This is perfect. Thanks @lijinf2!

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

One suggestion for improvement of the test code, but LGTM!

python/cuml/tests/dask/test_dask_logistic_regression.py Outdated Show resolved Hide resolved
Co-authored-by: Simon Adorf <sadorf@nvidia.com>
@csadorf
Copy link
Contributor

csadorf commented Oct 3, 2023

/merge

@rapids-bot rapids-bot bot merged commit 39dfc7e into rapidsai:branch-23.10 Oct 4, 2023
50 checks passed
@lijinf2 lijinf2 deleted the fea_lrmg_l1 branch June 26, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants