[MRG+1] raising warning for class_weight in fit method #3931
Conversation
|
I would have remove the param for consistency with other estimators @amueller wdyt? |
|
I think I agree with you actually. The class_weight seems like a parameter that should be kept to being passed only through the constructor. |
|
Just pushed a new commit for the warning |
| warnings.warn("You are trying to set class_weight through the fit " | ||
| "method, which will not be possible in a later " | ||
| "version of scikit. Pass the class_weight into " | ||
| "the constructor instead.") |
There was a problem hiding this comment.
this should be a deprecation warning. Say in which version it will disappear and please say scikit-learn and not just scikit. There are many scikits.
|
@agramfort this does remove the parameter (in the next release), right? |
in 2 releases so v0.17
yes |
|
The last commit should make this merge-able, let me know if there's anything else I can do |
|
+1 for merge. |
|
this is an uncontroversial PR. Any one to click the green button after my +1? |
| warnings.simplefilter("always", DeprecationWarning) | ||
| clf.fit(X4, Y4, class_weight=1) | ||
| assert_true(warning_message == str(w[0].message)) | ||
| assert_true(issubclass(w[0].category, DeprecationWarning)) |
There was a problem hiding this comment.
Apparently, you want to check the exact message that is warned. To do that, you could add assert_warns_regex.
|
The title is misleading. ;-)
|
|
When comments are handled, +1 ! |
|
Alrighty, let me know if that last change looks good. Thanks @arjoly |
|
LGTM |
|
that makes 2 +1. merging. |
[MRG+1] raising warning for class_weight in fit method
| if class_weight is not None: | ||
| warnings.warn("You are trying to set class_weight through the fit " | ||
| "method, which will be deprecated in version " | ||
| "v0.17 of scikit-learn. Pass the class_weight into " |
There was a problem hiding this comment.
Shouldn't this have been 0.18?
This should solve #3928, unless we decide to go the route of warning.