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
[MRG] Fixed n_support_ attr for OneClassSVM and SVR #15099
Conversation
I mean it's better but ideally we'd fix that nonsense as well, right? |
that means we need a deprecation then. On master |
I would argue it's a bugfix. On master it's uninitialized memory values afaik. |
sklearn/svm/base.py
Outdated
@@ -484,6 +484,17 @@ def coef_(self): | |||
def _get_coef(self): | |||
return safe_sparse_dot(self._dual_coef_, self.support_vectors_) | |||
|
|||
@property | |||
def n_support_(self): | |||
check_is_fitted(self) |
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.
I think a @property
raising a NotFittedError
is kinda odd. Rather have an AttributeError
here.
sklearn/svm/libsvm.pyx
Outdated
cdef np.ndarray[np.int32_t, ndim=1, mode='c'] n_class_SV | ||
n_class_SV = np.empty(n_class, dtype=np.int32) | ||
copy_nSV(n_class_SV.data, model) | ||
if (svm_type == 0 or svm_type == 1): |
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.
Rm parentheses
Comments were addressed, mergning. Thanks for the reviews! |
Closes #14799
Closes #15034
Fixes #14774
@amueller I think this is enough?
(That doesn't fix the nonsense that the attribute has size 2 (i.e. 2 classes) for OneClass and SVR but that's a different issue)It does now