-
-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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+1] FIX report n_iter_ as at most max_iter, without always reducing by 1 #10723
Conversation
I pushed a minor change in the test that checks that the solver is lbfgs and that the scipy version is affected by this bug. Other than this, LGTM. |
I guess this is a fine approach too.
|
It would be nice to have a second opinion on this one. Maybe @rth, @glemaitre or @qinhanmin2014? |
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.
LGTM, +1 for faithfully report the result from scipy. I don't think users should blame scikit-learn if they get different results here.
doc/whats_new/v0.20.rst
Outdated
@@ -382,6 +382,11 @@ Linear, kernelized and related models | |||
underlying implementation is broken. Use :class:`linear_model.Lasso` instead. | |||
:issue:`9837` by `Alexandre Gramfort`_. | |||
|
|||
- :class:`linear_model.LogisticRegression` with ``solver='lbfgs'`` formerly |
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 doubt whether it belongs to API changes
, either Enhancements
or Bug fixes
seems fine from my side.
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.
Going from scikit-learn 0.19 to 0.20, logistic_regression.n_iter_
will change (unless you use scipy > 1.0.0). In this respect "API changes" feels like the right place.
It'd be good to have a third opinion, it could potentially break some user code, e.g. if someone was checking |
so you're suggesting we could set `n_iter_ == min(nit, max_iter)`? I
suppose this would be consistent with what's currently there, but it would
remain idiosyncratic behaviour of logistic
|
IMO it's fine to leave n_iter_ as reported by scipy. Because this can break user code (a bit of a edge case though maybe), I think it'd be good to have another opinion. |
Tricky one. If we don't want to have any issue we should cover this case. However, we could also expect that user set |
Yep ... Thinking a bit more about it, I think doing |
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.
LGTM. I think it's a better solution.
ping @jnothman @lesteve @glemaitre I quickly searched the codebase with from sklearn.linear_model import HuberRegressor
from sklearn.datasets import load_boston
X, y = load_boston(return_X_y=True)
reg = HuberRegressor(max_iter=1)
reg.fit(X, y)
print(reg.n_iter_)
# 2 |
I've patched that too, but am happy to roll it back if the consensus is
against.
…On 4 March 2018 at 18:51, Hanmin Qin ***@***.***> wrote:
ping @jnothman <https://github.com/jnothman> @lesteve
<https://github.com/lesteve> @glemaitre <https://github.com/glemaitre> I
quickly searched the codebase with git grep "'nit'". It seems that
HuberRegressor has the same problem. Should we fix that here?
from sklearn.linear_model import HuberRegressorfrom sklearn.datasets import load_boston
X, y = load_boston(return_X_y=True)
reg = HuberRegressor(max_iter=1)
reg.fit(X, y)print(reg.n_iter_)# 2
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10723 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz67e7uMCJaxwaag554R3S96cmdRYEks5ta5z5gaJpZM4SWXHe>
.
|
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.
LGTM. I'll give my +1 again.
sklearn/linear_model/logistic.py
Outdated
.. versionchanged:: 0.20 | ||
|
||
In SciPy <= 1.0.0 the number of lbfgs iterations may exceed | ||
``max_iter``. ``n_iter_`` will nowreport at most ``max_iter``. |
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.
nowreport -> now report
I pushed some minor tweaks. I'll merge this one when Travis is green. Thanks everyone, I feel like we reached a reasonable solution! |
Yay! Travis Cron passed! No more daily "fail" emails! |
Great stuff, thanks for tackling this one @jnothman! |
…aster See scikit-learn/scikit-learn#10723 This fixes the build of `scikitlearn` on master and nixos-unstable. The issue is originally an upstream issue (see scikit-learn/scikit-learn#10619) which was fixed on master and was mainly caused by changes to the environment. Closes NixOS#43466
…aster (#43483) See scikit-learn/scikit-learn#10723 This fixes the build of `scikitlearn` on master and nixos-unstable. The issue is originally an upstream issue (see scikit-learn/scikit-learn#10619) which was fixed on master and was mainly caused by changes to the environment. Closes #43466
Fix #10619