-
Notifications
You must be signed in to change notification settings - Fork 544
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
[REVIEW] Multiclass meta estimator wrappers and multiclass SVC #3092
Conversation
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
4ccb764
to
1e32cc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have one question regarding object consumption
10e3bdb
to
a197b14
Compare
I have improved the docstring. Additionally I have remove the |
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #3092 +/- ##
===============================================
+ Coverage 71.45% 71.56% +0.10%
===============================================
Files 205 207 +2
Lines 16594 16702 +108
===============================================
+ Hits 11858 11952 +94
- Misses 4736 4750 +14
Continue to review full report at Codecov.
|
Fixed failing tests:
|
rerun tests |
1 similar comment
rerun tests |
rerun tests There seem to be on intermittent error with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a couple of suggestions regarding estimator usage, documentation, and decorators but overall it's nothing major.
I think before this gets merged you need to add the classes to the API doc so these new estimators will show up in our documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks at @mdemoret-nv for the review, I appreciate your suggestions, very useful! I have addressed the issues.
I have added a link to the API doc (f36f2d5), probably @JohnZed should have a look if that is the right place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @mdemoret-nv for the comments, I have fixed the remaining issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look good. Changes LGTM
This PR closes #959.
This PR implements multi class classification meta estimators, and multi class SVC.
The multiclass meta estimators are just thin wrappers around the scikit-learn counterpart to provide array conversion.
This has some overhead, but should be small compared to SVM training time. More info in issue #2876.
Limitations compared to scikit-learn multiclass SVC:
sample_weight
in multi class case, and a warning is issued. To handle that we would need to fork the multi class meta estimators and ensure that the class weight parameter is propagated to the binary classifier.decision_function_shape
arg not supportedbreak_ties
arg not supportedUsing sklearn.multiclass allows us to choose between one-vs-one and one-vs-rest classifiers. I have introduced an extra arg
multiclass_strategy
to let the user choose. One vs one is more accurate, but more costly.This is marked as work in progress, because the actual way of mapping back and fort to numpy data types (in order to make use of sklearn.multiclass) is going be simplified by #3040. Ideally we should merge this after #3040, but if that gets blocked we can bring this forward.