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

support parameter 'class_weight' and method 'decision_function' in LinearSVC #5364

Merged
merged 20 commits into from
May 10, 2023
Merged

support parameter 'class_weight' and method 'decision_function' in LinearSVC #5364

merged 20 commits into from
May 10, 2023

Conversation

mfoerste4
Copy link
Contributor

@mfoerste4 mfoerste4 commented Apr 17, 2023 •
edited
Loading

  • added support for 'class_weight' parameter to LinearSVC as already supported by SVC
  • add decision_function to LinearSVC

Should be non-breaking.
CC @tfeher

@mfoerste4 mfoerste4 requested review from a team as code owners April 17, 2023 14:54
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Apr 17, 2023
tfeher
tfeher requested changes Apr 17, 2023
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 Malte for the PR, apart from an issue with handling the multiclass case, it looks good overall. Please add a python unit test for the added features.

cpp/src/svm/linear.cu Outdated Show resolved Hide resolved
cpp/include/cuml/svm/linear.hpp Outdated Show resolved Hide resolved
python/cuml/svm/linear.pyx Outdated Show resolved Hide resolved
@tfeher tfeher added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 17, 2023
@mfoerste4 mfoerste4 requested a review from tfeher April 18, 2023 20:53
dantegd
dantegd requested changes Apr 19, 2023
View reviewed changes
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Overall the PR looks great! Will wait for @tfeher re-review and had small questions, but very glad to see this!

python/cuml/tests/test_linear_svm.py Show resolved Hide resolved
python/cuml/tests/test_linear_svm.py Outdated Show resolved Hide resolved
python/cuml/tests/test_linear_svm.py Outdated Show resolved Hide resolved
python/cuml/svm/svc.pyx Show resolved Hide resolved
@dantegd
Copy link
Member

dantegd commented Apr 19, 2023

Note CI issues will be resolved after #5371 is merged

tfeher
tfeher approved these changes Apr 20, 2023
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 Malte for addressing the issues so far! Apart from the pending issues from Dante, the PR looks good to me!

cpp/include/cuml/svm/linear.hpp Outdated Show resolved Hide resolved
python/cuml/tests/test_linear_svm.py Outdated Show resolved Hide resolved
@mfoerste4 mfoerste4 requested a review from dantegd April 20, 2023 10:50
csadorf
csadorf requested changes Apr 25, 2023
View reviewed changes
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.

Thank you for the contribution! I have a few comments and questions, but no major concerns except for a seemingly erroneous type check.

python/cuml/svm/linear.pyx Show resolved Hide resolved
python/cuml/svm/linear.pyx Show resolved Hide resolved
python/cuml/svm/linear.pyx Outdated Show resolved Hide resolved
python/cuml/svm/svc.pyx Outdated Show resolved Hide resolved
python/cuml/svm/svc.pyx Show resolved Hide resolved
@mfoerste4 mfoerste4 requested a review from csadorf April 26, 2023 09:45
@mfoerste4
Copy link
Contributor Author

@dantegd , have all your requests been addressed?

@tfeher
Copy link
Contributor

tfeher commented May 3, 2023

rerun tests

@dantegd
Copy link
Member

dantegd commented May 8, 2023

/merge

@rapids-bot rapids-bot bot merged commit 620a21e into rapidsai:branch-23.06 May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantegd dantegd dantegd approved these changes

@csadorf csadorf csadorf approved these changes

@tfeher tfeher tfeher approved these changes

Assignees
No one assigned
Labels
CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mfoerste4 @dantegd @tfeher @csadorf