[MRG+1] FIX report n_iter_ as at most max_iter, without always reducing by 1 by jnothman · Pull Request #10723 · scikit-learn/scikit-learn

jnothman · 2018-02-28T09:48:41Z

lesteve · 2018-02-28T14:48:13Z

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.

jnothman · 2018-02-28T19:33:44Z

I guess this is a fine approach too.

lesteve · 2018-03-01T08:19:37Z

It would be nice to have a second opinion on this one. Maybe @rth, @glemaitre or @qinhanmin2014?

qinhanmin2014

LGTM, +1 for faithfully report the result from scipy. I don't think users should blame scikit-learn if they get different results here.

qinhanmin2014 · 2018-03-01T09:11:35Z

doc/whats_new/v0.20.rst

  underlying implementation is broken. Use :class:`linear_model.Lasso` instead.
  :issue:`9837` by `Alexandre Gramfort`_.

+- :class:`linear_model.LogisticRegression` with ``solver='lbfgs'`` formerly


I doubt whether it belongs to API changes, either Enhancements or Bug fixes seems fine from my side.

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.

qinhanmin2014 · 2018-03-01T09:32:33Z

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.

Fair enough. @lesteve @jnothman merge?

lesteve · 2018-03-01T15:30:00Z

It'd be good to have a third opinion, it could potentially break some user code, e.g. if someone was checking logistic_regression.n_iter_ == logistic_regression.max_iter to figure out whether max_iter was reached before converging.

jnothman · 2018-03-01T21:26:34Z

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

lesteve · 2018-03-02T10:12:50Z

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.

glemaitre · 2018-03-02T15:54:10Z

logistic_regression.n_iter_ == logistic_regression.max_iter

Tricky one. If we don't want to have any issue we should cover this case.

However, we could also expect that user set verbose=1 and catch the ConvergenceWarning from there. This should be one of the reason to raise those warnings, isn't it :) Not sure this a good excuse to go around the issue.

lesteve · 2018-03-02T16:41:04Z

Tricky one.

Yep ...

Thinking a bit more about it, I think doing n_iter_ = min(n_iter_, max_iter) is the least worse option. At least we maintain a bit of internal consistency within scikit-learn (i.e. we keep n_iter_ <= max_iter). Obviously n_iter_ is still going to be wrong (in scipy <= 1.0.0).

This reverts commit 918e008.

…0619

qinhanmin2014

LGTM. I think it's a better solution.

qinhanmin2014 · 2018-03-04T07:51:19Z

ping @jnothman @lesteve @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 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

jnothman · 2018-03-04T09:30:48Z

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> .

qinhanmin2014

LGTM. I'll give my +1 again.

qinhanmin2014 · 2018-03-04T09:48:03Z

sklearn/linear_model/logistic.py

+        .. 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``.


nowreport -> now report

lesteve · 2018-03-05T09:38:11Z

I pushed some minor tweaks. I'll merge this one when Travis is green. Thanks everyone, I feel like we reached a reasonable solution!

jnothman · 2018-03-07T00:27:08Z

Yay! Travis Cron passed! No more daily "fail" emails!

lesteve · 2018-03-07T09:31:18Z

Great stuff, thanks for tackling this one @jnothman!

…cikit-learn#10723)

…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 report n_iter_ faithfully to scipy.optimize

918e008

jnothman changed the title ~~[MRG] FIX report n_iter_ faithfully to scipy.optimize~~ [MRG] FIX report n_iter_ in accordance with scipy.optimize Feb 28, 2018

jnothman added the Bug label Feb 28, 2018

jnothman requested a review from lesteve February 28, 2018 11:58

Only relax the test for lbfgs solver and specific scipy versions

024c7f9

qinhanmin2014 approved these changes Mar 1, 2018

View reviewed changes

qinhanmin2014 changed the title ~~[MRG] FIX report n_iter_ in accordance with scipy.optimize~~ [MRG+1] FIX report n_iter_ in accordance with scipy.optimize Mar 1, 2018

jnothman added 4 commits March 4, 2018 08:28

Revert "FIX report n_iter_ faithfully to scipy.optimize"

75fec50

This reverts commit 918e008.

FIX report min(n_iter, max_iter) as n_iter_ in lbfgs

77075b7

Merge branch 'fix10619' of github.com:jnothman/scikit-learn into fix1…

fb6c95e

…0619

Complete reversion

e87cedc

jnothman changed the title ~~[MRG+1] FIX report n_iter_ in accordance with scipy.optimize~~ [MRG] FIX report n_iter_ as at most max_iter, without always reducing by 1 Mar 3, 2018

qinhanmin2014 approved these changes Mar 4, 2018

View reviewed changes

make the huber case consistent

9cc6593

qinhanmin2014 approved these changes Mar 4, 2018

View reviewed changes

qinhanmin2014 changed the title ~~[MRG] FIX report n_iter_ as at most max_iter, without always reducing by 1~~ [MRG+1] FIX report n_iter_ as at most max_iter, without always reducing by 1 Mar 4, 2018

jnothman and others added 2 commits March 4, 2018 21:11

DOC missing space

1bc5542

Minor tweaks

b125afe

lesteve merged commit 2aba6e2 into scikit-learn:master Mar 5, 2018

glemaitre mentioned this pull request Mar 15, 2018

Mixture modeling reports wrong number of n_iter_ #10740

Closed

jnothman mentioned this pull request Jul 4, 2018

MNT Release 0.19.2 with python 3.7 support #11422

Merged

jnothman added a commit to jnothman/scikit-learn that referenced this pull request Jul 4, 2018

FIX n_iter_ should be less than max_iter in when using lbfgs solver(s…

76e2491

…cikit-learn#10723)

Ma27 mentioned this pull request Jul 13, 2018

pythonPackages.scikitlearn: apply max_iter patch from scikitlearn master NixOS/nixpkgs#43483

Merged

9 tasks

Uh oh!

Conversation

jnothman commented Feb 28, 2018

Uh oh!

lesteve commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman commented Feb 28, 2018 via email

Uh oh!

lesteve commented Mar 1, 2018

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

lesteve Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 commented Mar 1, 2018

Uh oh!

lesteve commented Mar 1, 2018

Uh oh!

jnothman commented Mar 1, 2018 via email

Uh oh!

lesteve commented Mar 2, 2018

Uh oh!

glemaitre commented Mar 2, 2018

Uh oh!

lesteve commented Mar 2, 2018

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 commented Mar 4, 2018

Uh oh!

jnothman commented Mar 4, 2018 via email

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 Mar 4, 2018

Choose a reason for hiding this comment

Uh oh!

lesteve commented Mar 5, 2018

Uh oh!

jnothman commented Mar 7, 2018

Uh oh!

lesteve commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lesteve commented Feb 28, 2018 •

edited

Loading