-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
RFC LogisticRegression code is way to opaque #11865
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
Comments
Also, using the term 'ovr' in the binary case (which after #11859 will be everywhere) is very confusing. |
I'll take this on. |
Not yet, you won't! It's undergoing active changes. Please try again in a
couple of weeks.
|
Awesome; thanks for letting me know! |
I agree with the assessment of the code being opaque. Are there any more actionable insights except for better naming of |
Trying to improve some things a bit for linear models, I got stuck in the swamp of If I were to rewrite it without keeping legacy, I would do:
@scikit-learn/core-devs ping for discussion. |
Is this one of those things like what we did with I like the ideas you have here, but I'm not sure why the parallelism needs to go away or why it's not helpful. I think it'd be nice to have a maintainable code there. |
@lorentzenchr The changes that you are proposing seem so significant that, should we go in this direction, it is probably better to ramp up a new object to ensure smooth transition to our users (rather than an abrupt break of backward compatibility). However, I feel that most of the changes that you propose are net loss of useful functionality for the user. With regards to the penalization parametrization, I clearly agree with you, and there are several places in scikit-learn where I would like to change how we parametrize our penalties (for instance, in the ridge, it should account for a norm of the design matrix, in order to have good defaults). One question is: how to do this smoothly? It can be addressed, but I am not sure that it will simplify the code. With regards to ovr, I think that it is a actually an important aspect of the objects, in particular the CV one. In my experience, in multiclass one often needs different penalty per class. The reason is for instance that the prevalence of the classes vary widely. One could argue that the use of the metaclassifier OneVsRestClassifier means that the corresponding functionality can be implemented via a pipeline. But it's much harder to implement. Having good patterns easy to implement for the user seems like a priority. With regards to parallelism, I actually think that parallelism is something very important, and that we should be working on having more parallelism in scikit-learn. Modern computers have many cores (my laptop has 8, many compute nodes have much more). |
After the release we should refactor the logistic regression code.
Right now, path computation, optimizers and OVR are all a tangled mess.
The text was updated successfully, but these errors were encountered: