I mentioned this in the previous issue: is n_iter ever set to 0 for k-means, or is it strictly greater than 0? Because I would see how this assignment might be useful if the subsequent for loop does not assignment them.

It looks contingent on n_samples, self. max_iter and self. batch_size

So it appears very unlikely that n_iter would ever be set to 0.

Even if n_iter=0, the variables batch_inertia, centers_squared_diff don't seem to be used afterwards, so it should be indeed fine.

If n_samples is not needed, then it's also not needed here.

Doesn't this look odd? The comment says it's trying a smaller matrix, but it sets the n_best_tot after retrying the matrix allocation. It seems to me that those lines are swapped somehow.

Good point. I'll try swapping the lines and see if anything breaks.

Shouldn't it print the exception instead of ignoring it? It kinda feels odd to catch Exception and suppress it completely.

There's a raise statement after the print statement below, so the exception will still be visible. It's still strange semantics that the exception does not wrap the print message though.

There's a raise, sure. But it says that the estimator is not failing properly on sparse data, i.e. the message is being very specific, whereas it's catching Exception. So if the estimator fails because there's not enough memory, or because there's something wrong with the disk and the estimator reads from the disk while fitting, for whatever reason, we still show the same error message. It's just a bad idea to catch Exception, and pretend it's much more specific than Exception.

Agreed that it's bad that except Exception can be catching anything (minus a TypeError or ValueError from above) while a specific print message is being shown (should probably be a warning).

I am not too familiar with this part of the codebase, but the scope of this PR (just removing unused variables) does not change the prior behavior. This block can be improved in a future PR.

In [5]: try:
   ...:     1 / 0
   ...: except Exception as e:
   ...:     print('Something went wrong')
   ...:     raise
   ...:
   ...:
Something went wrong
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
<ipython-input-5-b91504a88660> in <module>()
      1 try:
----> 2     1 / 0
      3 except Exception as e:
      4     print('Something went wrong')
      5     raise

ZeroDivisionError: division by zero

In [6]: try:
   ...:     1 / 0
   ...: except Exception:
   ...:     print('Something went wrong')
   ...:     raise
   ...:
   ...:
Something went wrong
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)
<ipython-input-6-ee8d63f9937e> in <module>()
      1 try:
----> 2     1 / 0
      3 except Exception:
      4     print('Something went wrong')
      5     raise

ZeroDivisionError: division by zero

@mroeschke , sorry, hadn't noticed the raise after the print. It's all good. Thanks!

I was like "this definitely doesn't look right" but it actually surprisingly is. With estimator tags we could entirely remove this line.

Not familiar with estimator tags, but are the tags in place so this line can be removed or should this be kept for now to still validate X?

should be kept for now, sorry for being cryptic.

Even if n_iter=0, the variables batch_inertia, centers_squared_diff don't seem to be used afterwards, so it should be indeed fine.

It's a backport from scipy, it's better to keep it without change, which will make comparing with the upstream version easier.