[MRG + 1] Flip sign of SVM's dual_coef_ in a binary case #4326
Conversation
Makes it consistent with other sign adjustments, adds a test to reproduce decision_function in a kernel case
| assert_array_equal(clf.predict([[-.1, -.1]]), [1]) | ||
| clf.dual_coef_ = np.array([[.0, 1.]]) | ||
| clf._dual_coef_ = np.array([[.0, 1.]]) |
There was a problem hiding this comment.
Note this. Since this PR fixes sign of dual_coef_ in the same way intercept_ is fixed (which is done by introducing another property _intercept_), then it'd be a bit non-intuitive to set new dual coefficients, since a user would need to change _dual_coef_ instead of dual_coef_ (Should I document it somewhere?).
Though, this usecase seems very odd and rare to me in the first place, I can't imagine a case when I'd need to change dual coefficients.
There was a problem hiding this comment.
We could forbid overwriting dual_coef_ as we did with coef_. I'm not sure that is necessary, though.
There was a problem hiding this comment.
I am not entirely sure this test tests something very useful ;)
|
This looks good to me. Please document the change in whatsnew, and also feel free to change the documentation mentioning -rho. |
| clf.fit(X, Y) | ||
| norm = lambda x: np.apply_along_axis(np.linalg.norm, 1, x) | ||
| rbfs = [np.exp(-clf.gamma * norm(clf.support_vectors_ - X[i])**2) | ||
| for i in range(len(X))] |
There was a problem hiding this comment.
Couldn't you use:
from sklearn.metrics.pairwise import rbf_kernel
rbfs = rbf_kernel(X, clf.support_vectors_, gamma=clf.gamma)instead?
There was a problem hiding this comment.
Didn't know about this function, edited.
| @@ -181,8 +181,11 @@ def fit(self, X, y, sample_weight=None): | |||
| # In binary case, we need to flip the sign of coef, intercept and | |||
| # decision function. Use self._intercept_ internally. | |||
There was a problem hiding this comment.
Please update the comment to tell "Use self.intercept and self.dual_coef internally".
|
LGTM, thanks for the fix @Barmaley-exe. +1 once my last minor comment is addressed. |
[MRG + 1] Flip sign of SVM's dual_coef_ in a binary case
|
Merged, thanks! |
This PR aims to fix #4262
For the details, please refer to the issue discussion.