-
-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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
MNT replace log_logistic by logaddexp #27544
Conversation
I set the label "no changelog needed" as it seems off to add an efficiency entry in whatsnew to the neural net module. One might get the wrong impression. |
See the blog post describing this implementation: | ||
http://fa.bianp.net/blog/2013/numerical-optimizers-for-logistic-regression/ | ||
""" | ||
is_1d = X.ndim == 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.
This function was still "kind" of public. It can internally call np.logaddexp
now.
Can we deprecate it and asking to use the numpy one and then we remove it in 2 versions.
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.
But it is not listed in our API docs. So it is not public.
Do you know of any user/3rd party using that function from us?
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.
https://github.com/search?q=log_logistic+language%3APython&type=code&l=Python&p=1 gives 904 hits, most of which are forks of scikit-learn. For me, that's strong evidence that we can remove. I can add a changelog and say with what function to replace it.
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.
But it is not listed in our API docs.
Yes, we have some legacy and bad practices that should be fixed. We should have clear public, developer, and private tools.
So it is not public.
This is tricky because the public/private agreement would be the presence or not of a leading _
.
Do you know of any user/3rd party using that function from us?
Apparently some do: https://github.com/search?q=from+sklearn.utils.extmath+import+log_logistic&type=code
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.
For instance, this is one third-party library: https://github.com/VowpalWabbit/vowpal_wabbit with 8.3k stars and 2k forks. Removing without notice will make a lot of people angry at once :)
I am still advocating for a deprecation. I know that this is frustrating for such a tools that should have never been public but this is safer.
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.
About vowpalwabbit, their only use is np.exp(log_logistic(...))
which is just crazy.
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.
Keep it with wrapping logaddexp and deprecate it?
Yes.
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.
Not happy but will do.
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.
As you see, that's not a 5 minutes thing. It cost me 1/2h.
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.
But that prevented potential breakages which my hasty approving review definitely had not envisioned. 🤦
Thank you for the efforts, @lorentzenchr.
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.
Thanks @lorentzenchr Merging.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
No
What does this implement/fix? Explain your changes.
This PR replaces
log_logistic(x)
by the more precise and faster-np.logaddexp(0, -x)
.The whole Cython module
_logistic_sigmoid
is therefore removed.Any other comments?