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

Enable multiclass svm for sparse input #5588

Merged

Conversation

mfoerste4
Copy link
Contributor

This commit enables multiclass SVM for sparse input. Previously this was deactivated as the input_to_host_array functionality does not support sparse arrays, but the data has to be piped through sklearn classes which requires host data.

@dantegd , this is a local workaround to enable a input_to_host_array for sparse data without the complexity of providing the whole functionality of that function. Please have a look whether this is an acceptable solution for this use case.

FYI, @tfeher

@mfoerste4 mfoerste4 requested a review from a team as a code owner September 27, 2023 11:44
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Sep 27, 2023
@dantegd dantegd added the 3 - Ready for Review Ready for review by team label Oct 2, 2023
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, it looks good overall. Just one question about testing: do we have only the pickle test for this feature? If yes, could we improve coverage in test_svm.py?

python/cuml/multiclass/multiclass.py Outdated Show resolved Hide resolved
python/cuml/multiclass/multiclass.py Outdated Show resolved Hide resolved
python/cuml/svm/svc.pyx Outdated Show resolved Hide resolved
@mfoerste4
Copy link
Contributor Author

Thanks Malte for the PR, it looks good overall. Just one question about testing: do we have only the pickle test for this feature? If yes, could we improve coverage in test_svm.py?

@tfeher , I extended an existing multiclass test to also use sparse inputs.

@csadorf csadorf added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 11, 2023
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.

Just one request, otherwise looks good. Make sure to update the base branch to branch-23.12!

python/cuml/multiclass/multiclass.py Outdated Show resolved Hide resolved
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 @mfoerste4 for adding the test! Apart from the last open question about the helper placement the PR looks good to me!

@mfoerste4 mfoerste4 changed the base branch from branch-23.10 to branch-23.12 October 16, 2023 09:13
@mfoerste4
Copy link
Contributor Author

Just one request, otherwise looks good. Make sure to update the base branch to branch-23.12!

@csadorf, I believe the failing CI checks are unrelated to this PR. Could you have a look at the changes requested?

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.

LGTM!

@csadorf
Copy link
Contributor

csadorf commented Nov 2, 2023

/merge

@csadorf csadorf added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Nov 8, 2023
@csadorf
Copy link
Contributor

csadorf commented Nov 14, 2023

@mfoerste4 @tfeher I think the cudaErrorMisalignedAddress:misaligned address errors we are seeing in the wheel tests are actually not a fluke, but related to the change set in this PR. I don't have a hypothesis yet as to what might be causing it.

@csadorf csadorf added 4 - Waiting on Author Waiting for author to respond to review and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels Nov 14, 2023
@mfoerste4 mfoerste4 requested a review from a team as a code owner November 15, 2023 19:07
@csadorf
Copy link
Contributor

csadorf commented Nov 15, 2023

@mfoerste4 Thanks for the fix. Tests are currently expected to fail because of an upstream incompatibility between raft and rmm.

@csadorf csadorf added 4 - Waiting on Reviewer Waiting for reviewer to review or respond 0 - Blocked Cannot progress due to external reasons and removed 4 - Waiting on Author Waiting for author to respond to review labels Nov 15, 2023
@mfoerste4
Copy link
Contributor Author

@mfoerste4 Thanks for the fix. Tests are currently expected to fail because of an upstream incompatibility between raft and rmm.

@csadorf , I was not able to reproduce the issue locally, I just hope it is a duplicate of the bug that i came across yesterday.

@rapids-bot rapids-bot bot merged commit 1570ed7 into rapidsai:branch-23.12 Nov 21, 2023
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Blocked Cannot progress due to external reasons 4 - Waiting on Reviewer Waiting for reviewer to review or respond 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