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.

Sign up for GitHub

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

Jump to bottom

LinearSVM using QN solvers #4268

Merged
merged 55 commits into from
Nov 17, 2021
Merged

LinearSVM using QN solvers #4268

merged 55 commits into from
Nov 17, 2021

Conversation

achirkin
Copy link
Contributor

@achirkin achirkin commented Oct 5, 2021

Implementing LinearSVM using the existing QN solvers.

@achirkin achirkin self-assigned this Oct 5, 2021
@github-actions github-actions bot added CMake CUDA/C++ Cython / Python Cython or Python issue labels Oct 5, 2021
@achirkin achirkin requested a review from tfeher October 5, 2021 11:35
@achirkin achirkin added 2 - In Progress Currenty a work in progress non-breaking Non-breaking change feature request New feature or request labels Oct 5, 2021
@achirkin
Copy link
Contributor Author

achirkin commented Oct 5, 2021

This PR would address #2773 and possibly #1664. Though I am not sure whether we should automatically switch to fast LinearSVM solver when general SVM is called with the linear kernel 🤔 .

@caryr35 caryr35 added this to PR-WIP in v21.12 Release via automation Oct 6, 2021
dantegd
dantegd requested changes Oct 27, 2021
View reviewed changes
python/cuml/svm/linear_svc.py Outdated Show resolved Hide resolved
v21.12 Release automation moved this from PR-WIP to PR-Needs review Oct 27, 2021
tfeher
tfeher reviewed Nov 10, 2021
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @achirkin for updating the PR. I have a few more smaller comments.

cpp/src/glm/qn/glm_base.cuh Outdated Show resolved Hide resolved
cpp/test/sg/linear_svm_test.cu Show resolved Hide resolved
cpp/src/svm/linear.cu Outdated Show resolved Hide resolved
cpp/src/svm/linear.cu Outdated Show resolved Hide resolved
cpp/src/svm/linear.cu Outdated Show resolved Hide resolved
cpp/test/sg/linear_svm_test.cu Outdated Show resolved Hide resolved
python/cuml/svm/linear.pyx Show resolved Hide resolved
python/cuml/svm/linear_svc.py Outdated Show resolved Hide resolved
python/cuml/svm/linear_svr.py Show resolved Hide resolved
python/cuml/svm/linear_svc.py Outdated Show resolved Hide resolved
@achirkin achirkin requested a review from tfeher November 15, 2021 10:42
tfeher
tfeher approved these changes Nov 15, 2021
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @achirkin for fixing the issues. The C++ side looks good to me (apart from a small issue that I trust you will fix) therefore pre-approving.

On the Python side there is still one point open, where we could improve consistency with the sklearn API.

cpp/src/svm/linear.cu Outdated Show resolved Hide resolved
python/cuml/svm/linear_svc.py Outdated Show resolved Hide resolved
robertmaynard
robertmaynard approved these changes Nov 15, 2021
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

CMake changes LGTM

@tfeher tfeher added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Nov 17, 2021
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.12@b5f119e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.12    #4268   +/-   ##
===============================================
  Coverage                ?   85.72%           
===============================================
  Files                   ?      234           
  Lines                   ?    19175           
  Branches                ?        0           
===============================================
  Hits                    ?    16437           
  Misses                  ?     2738           
  Partials                ?        0
Flag Coverage Δ
dask 46.68% <0.00%> (?)
non-dask 78.56% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5f119e...ea0606c. Read the comment docs.

v21.12 Release automation moved this from PR-Needs review to PR-Reviewer approved Nov 17, 2021
@dantegd
Copy link
Member

dantegd commented Nov 17, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit f30e529 into rapidsai:branch-21.12 Nov 17, 2021
v21.12 Release automation moved this from PR-Reviewer approved to Done Nov 17, 2021
@achirkin achirkin deleted the fea-linear-svm branch November 18, 2021 06:44
@achirkin achirkin mentioned this pull request Nov 18, 2021
Open
This was referenced Nov 18, 2021
Closed
Closed
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@achirkin
LinearSVM using QN solvers (rapidsai#4268)
f4a851c 
Implementing LinearSVM using the existing QN solvers.

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Tamas Bela Feher (https://github.com/tfeher)
  - Robert Maynard (https://github.com/robertmaynard)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4268
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lowener lowener lowener requested changes

@robertmaynard robertmaynard robertmaynard approved these changes

@dantegd dantegd dantegd approved these changes

@tfeher tfeher tfeher approved these changes

Assignees

@achirkin achirkin

Labels
3 - Ready for Review Ready for review by team CMake CUDA/C++ Cython / Python Cython or Python issue feature request New feature or request non-breaking Non-breaking change
Projects
No open projects
v21.12 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@achirkin @lowener @codecov-commenter @dantegd @robertmaynard @tfeher