8000 add **kwargs to sklearn.multiclass.(OneVsOne,OneVsRest).fit(X,y) · Issue #11956 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

add **kwargs to sklearn.multiclass.(OneVsOne,OneVsRest).fit(X,y) #11956

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
Closed
azrdev opened this issue Aug 31, 2018 · 4 comments
Closed

add **kwargs to sklearn.multiclass.(OneVsOne,OneVsRest).fit(X,y) #11956

azrdev opened this issue Aug 31, 2018 · 4 comments

Comments

@azrdev
Copy link
azrdev commented Aug 31, 2018

Description

I'm implementing a binary classifier. To support multi-class problems, I copied the approach from Gaussian process classification: if n_classes > 2, wrap the binary classifier in a OneVsOneClassifier or OneVsRestClassifier.

However, this is incompatible with me adding additional arguments to fit(), because the wrappers don't forwards these arguments to the wrapped binary classifier.

AIUI the fix would be to change all fit(X, y) methods in sklearn.multiclass to fit(X, y, **kwargs), and pass that new argument to the wrapped.fit(X, y, **kwargs) call.

Should I simply create a merge request, or am I missing something?

Versions

Linux-4.18.5-arch1-1-ARCH-x86_64-with-arch
Python 3.7.0 (default, Jul 15 2018, 10:44:58) 
[GCC 8.1.1 20180531]
NumPy 1.15.0
SciPy 1.1.0
Scikit-Learn 0.19.2
@adrinjalali
Copy link
Member

This relates to #4497 and #9566. It is unfortunately not as straightforward as one would think, considering all the issues discussed in those threads.

@jnothman
Copy link
Member
jnothman commented Sep 1, 2018 via email

@azrdev
Copy link
Author
azrdev commented Jan 29, 2019

seems like this had already been reported as #10882

@jnothman
Copy link
Member

Yes, it seems it has. That issue deserves a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jnothman @adrinjalali @azrdev
0