Is the default max_iter something we should by changing while we are making other backwards incompatible changes? Or is the current default reasonable

To be honest, I don't know. The ConvergenceWarning is partly there to allow users to adjust max_iter depending on their problem.

We can only make this change if we include it in the 0.19 final release. Just to note...

I am not sure I follow so I'll rephrase what I understood:

If we create a separate PR just with this change, it can get included in the 0.19 final release.

Did I understand the note correctly? If so, I'll create one presently.

Yes :)

I think this is right given the current implementation, but the current implementation is buggy in that it seems you can never obtain n_iter_==max_iter.

Please fix the convergence condition and inline the code for _not_converged. I would then write something like:

while self.n_iter_ < self.max_iter:
    if ... < tol:
        break
    Do stuff
    self.n_iter_ += 1
else:
    Warn

Done.

I suppose we could now do this as a for loop...

You mean setting self.n_iter_ using the loop variable?
I personally feel a bit uncomfortable using the loop variable outside the loop.

(* angsty feeling *)

So ... shall I make this change to expedite the merge?

I think he meant

for n_iter in range(self.max_iter):

With the for loop, there are two alternatives:

Alternative 1

Setting self.n_iter_ after the for loop. It would look a bit ugly because we'll need to figure out whether in the last iteration the loop the tolerance condition was met (then self.n_iter_ = n_iter) or the else: branch was reached (in which case self.n_iter_ = n_iter + 1):

self.n_iter_ = 0
for n_iter in range(self.max_iter):
    if converged:
        break
    # ...
else:
    # warn
    self.n_iter_ = 1 # Count the last iteration.

self.n_iter_ += n_iter

Alternate 2

Leaving self.n_iter_ += 1 inside the for-loop. In this case case the loop variable (i.e. n_iter) is not used anywhere.

self.n_iter_ = 0
for n_iter in range(self.max_iter):
    if converged:
        break
    # ...
    self.n_iter_ += 1
else:
    # warn

Were any of these versions what you (both of you) had in mind? Personally, I like the while loop and then Alternative 2. :)

@amueller In the new proposal, self.n_iter_ can never be zero, and zero is a valid value technically. The handling of these corner cases is what makes the for loop slightly ugly.

I'm sorry, I am away from my laptop and am a bit constrained in my explanations.

This comment was a cosmetic one that is not worth withholding merge for.

So ... is that a "go" for the while loop? :-)

Whatever you think most readable. I'd prefer

for self.n_iter_ in ...:
   ...
else:
   ...

I think, but it matters very little

First, I didn't know that we could use self.n_iter_ in the for loop; today I learned. :)

Second, this is the best implementation I can think of which has the same behavior as the original while loop:

for self.n_iter_ in range(max_iter):
    # ...
else:
    # ...
    self.n_iter_ += 1

The self.n_iter_ += 1 in the else: clause is necessary to ensure that self.n_iter_ == self.max_iter is possible and that it correctly happens if the loop doesn't break out due to convergence. This was the issue we were out to fix originally. :)

With this implementation, I am quite happy with the for loop as well. I'll make this change. 👍