Fix LogisticRegression use with labels of string dtype
#6328
Conversation
| self.classes__, y = np.unique(y, return_inverse=True) | ||
| self.numeric_classes_ = np.arange(len(self.classes__)) |
There was a problem hiding this comment.
What is the motivation for introducing new estimator attributes? I'd suggest that we just convert the provided labels into numeric classes and drop them otherwise.
There was a problem hiding this comment.
The motivation was to reproduce the behavior of a Scikit-Learn estimator :
>>> print(cuLogRegModel.classes_)
['setosa' 'versicolor' 'virginica']
But, to be frank this is a small detail and making it work involves solving a lot of edge cases in CI. Since we need this to be merged very soon, it is safer to go with a simpler version that just converts the inputs to numeric classes.
There was a problem hiding this comment.
I'm not sure that it is such a small detail, but I'd support merging the simpler version and leaving this as a follow-up.
There was a problem hiding this comment.
Why not use the LabelEncoder to handle all this complexity? It comes with the right value in its classes_ attribute "for free".
If we merge something that is different from what scikit-learn does we just create more problems for ourselves IMHO. We put in effort to a half baked fix, we have to deal with users who report breakage and then we need to do the "right thing" anyway later.
There was a problem hiding this comment.
The difficulty isn't in producing the unique classes, but to make sure that the numeric version of classes can be used internally by cuML while the text version can be displayed through the classes_ attribute. Just pushed a solution that might pass CI. Then, sure we can use the LabelEncoder estimator instead of the unique function if that helps, it could especially handle https://github.com/rapidsai/cuml-accel/issues/94.
There was a problem hiding this comment.
I don't have an implementation yet, was going to finish up my other issue first. Haven't done more than look at how scikit-learn does it (with a LabelEncoder). I am wary of the additional fitted attribute here (numeric_classes_ and classes__). The scikit-learn implementation doesn't need this extra state and manages to handle things just fine - it would be nice to avoid increasing the amount of state stored on the class if possible (but again, apologies, I haven't looked more into how we might achieve that).
|
I've opened #6346 as an alternative fix for this issue. That patch doesn't add any additional state, and I believe better handles matching the output type and dtype for |
No description provided.