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] Updated type of warnings about convergence (Fixes #10173) #10306

Merged
merged 4 commits into from Feb 10, 2018

Conversation

Projects
None yet
4 participants
@jotasi
Contributor

jotasi commented Dec 13, 2017

Reference Issues/PRs

Fixes #10173
See also #10200 which is an oldish and incomplete pull request that attempts to fix the same issue.

What does this implement/fix?

This pull request changes the UserWarnings, warning about convergence problems to ConvergenceWarnings. Additionally to the two functions that are mentioned in the issue report, all other such warnings, that I could find were updated.

(Hopefully not too stupid) questions

There were two cases were I was not sure whether a ConvergenceWarning would be the correct choice and I could not find any clarifying documentation:

  1. if self.alpha == 0:
    warnings.warn("With alpha=0, this algorithm does not converge "
    "well. You are advised to use the LinearRegression "
    "estimator", stacklevel=2)

    Here, the warning is about bad convergence of the method for this specific parameter value. Is this a proper use case for a ConvergenceWarning, or are those only for cases were a convergence actually failed?

  2. if (self.n_skips_no_inliers_ + self.n_skips_invalid_data_ +
    self.n_skips_invalid_model_) > self.max_skips:
    warnings.warn("RANSAC found a valid consensus set but exited"
    " early due to skipping more iterations than"
    " `max_skips`. See estimator attributes for"
    " diagnostics (n_skips*).",
    UserWarning)

    I do not know enough about this function to know whether this constitutes a failure to converge.

For two of the functions, tests were updated or added respectively. For the other functions, I was not aware of a simple way to find a way to reliably trigger said warnings, thus no tests have been added for those. Both, pointers in the right direction how to construct those cases or suggestions for test cases would be greatly appreciated.

@amueller

This comment has been minimized.

Member

amueller commented Dec 13, 2017

I think the first is not a convergence warning but the second is. Convergence warning should be something about the algorithm not finishing "properly", the first one is just parameter validation.

@@ -74,7 +75,8 @@ def _nipals_twoblocks_inner_loop(X, Y, mode="A", max_iter=500, tol=1e-06,
if np.dot(x_weights_diff.T, x_weights_diff) < tol or Y.shape[1] == 1:
break
if ite == max_iter:
warnings.warn('Maximum number of iterations reached')
warnings.warn('Maximum number of iterations reached',

This comment has been minimized.

@amueller

amueller Dec 13, 2017

Member

you can probably test this by setting max_iter=1.

This comment has been minimized.

@jotasi

jotasi Dec 14, 2017

Contributor

I've added a test with low max_iter and very low tol that triggers this ConvergenceWarning. The other two warnings about unchanged Y residuals or X scores are not tested, though.

@amueller

looking good so far. No idea how to test birch or the GP. birch because I'm unfamiliar, GP is tricky since we don't control the lbfgs-parameters. we could provide a super ill-conditioned problem, but since we can't set max_iter, it might take a long time to run?

@@ -428,7 +429,8 @@ def _constrained_optimization(self, obj_func, initial_theta, bounds):
fmin_l_bfgs_b(obj_func, initial_theta, bounds=bounds)
if convergence_dict["warnflag"] != 0:
warnings.warn("fmin_l_bfgs_b terminated abnormally with the "
" state: %s" % convergence_dict)
" state: %s" % convergence_dict,

This comment has been minimized.

@amueller

amueller Dec 13, 2017

Member

don't think we can test well for this?

This comment has been minimized.

@jotasi

jotasi Dec 14, 2017

Contributor

That was my feeling as well, both for the classifier and the regressor. While both have several test cases, neither tests that for a failed convergence a warning is given (at least as far as I can see) so this part is uncovered. Btw. even in scipy's unit tests of scipy.optimize.fmin_l_bfgs_b I did not find a test for a failed convergence. So I don't see a simple way of testing this, either.

This comment has been minimized.

@jnothman

jnothman Dec 17, 2017

Member

Well this code appears to have test coverage, so it's being triggered somewhere. Do a full test run and check the logs to find out where...?

This comment has been minimized.

@jotasi

jotasi Dec 18, 2017

Contributor

This is actually not covered, but the warning in sklearn/gaussian_process/gpr.py. See my answer below for the answer where it is triggered.

This comment has been minimized.

@jnothman

jnothman Dec 20, 2017

Member

The coverage tool seems to think this line is executed by CI. But the number of iterations is the large default, so I'm not sure what kind of data triggers this. I'm therefore okay with leaving it untested, but it's strange that it does seem to be covered somewhere/sometimes in testing, as you note.

@@ -461,7 +462,8 @@ def _constrained_optimization(self, obj_func, initial_theta, bounds):
fmin_l_bfgs_b(obj_func, initial_theta, bounds=bounds)
if convergence_dict["warnflag"] != 0:
warnings.warn("fmin_l_bfgs_b terminated abnormally with the "
" state: %s" % convergence_dict)
" state: %s" % convergence_dict,

This comment has been minimized.

@amueller

amueller Dec 13, 2017

Member

Don't know how to test well for this, but it's covered? Can you check how it's covered possibly?

@@ -716,7 +716,7 @@ def logistic_regression_path(X, y, pos_class=None, Cs=10, fit_intercept=True,
iprint=(verbose > 0) - 1, pgtol=tol)
if info["warnflag"] == 1 and verbose > 0:
warnings.warn("lbfgs failed to converge. Increase the number "
"of iterations.")
"of iterations.", ConvergenceWarning)

This comment has been minimized.

@amueller

amueller Dec 13, 2017

Member

can check with max_iter.

This comment has been minimized.

@jotasi

jotasi Dec 14, 2017

Contributor

A test was added, the problem before was that I overlooked the necessity to set verbose>0.

This comment has been minimized.

@amueller

amueller Dec 15, 2017

Member

That's actually odd. I'm not sure we want that behavior? hm...

This comment has been minimized.

@jotasi

jotasi Dec 16, 2017

Contributor

At least I was a little surprised by it. It's not only the behavior of logistic_regression_path though. ridge_regression also requires verbose>0 to warn about convergence problems.

This comment has been minimized.

@jnothman

jnothman Feb 10, 2018

Member

Could you create a new issue to discuss this behaviour? Thanks

@@ -73,7 +74,7 @@ def _mv(x):
if max_iter is None and info > 0 and verbose:
warnings.warn("sparse_cg did not converge after %d iterations." %
info)
info, ConvergenceWarning)

This comment has been minimized.

@amueller

amueller Dec 13, 2017

Member

can check with max_iter.

This comment has been minimized.

@jotasi

jotasi Dec 14, 2017

Contributor

A test was added here as well.

@jnothman

I've not reviewed yet, but on the face of it: nice work; good questions! I agree with Andy's judgements about what is and isn't convergence. Birch is already tested, isn't it?

@jotasi

This comment has been minimized.

Contributor

jotasi commented Dec 14, 2017

Thanks to both of you for your impressively quick replies and especially to @amueller for the extensive comments. I have changed the second case (ransac) to a ConvergenceWarning. Yes, @jnothman, Birch was already tested, so I only had to switch warning types in the test. I also added test cases for most other cases. Still uncovered are the Guassian process classifier and regressor, as well as two special cases in pls. See above why those are still missing.

@amueller

This comment has been minimized.

Member

amueller commented Dec 15, 2017

I'm fine with leaving those uncovered. How did you check that you haven't forgotten any?

@jotasi

This comment has been minimized.

Contributor

jotasi commented Dec 16, 2017

Ok, then I'll leave those cases uncovered.
I also added a ConvergenceWarning in cluster.affinity_propagation_ that I missed in the first round. What I did to find the warnings was to go through all files that contain a warnings.warn, as well as either "C/converge" or "I/iter". For each warnings.warn in these files I checked whether I thought that the warning was about a convergence problem. However, I'm not really familiar with the code base yet and thus it's entirely possible that I overlooked something.

@jotasi jotasi changed the title from [WIP] Updated type of warnings about convergence (Fixes #10173) to [MRG] Updated type of warnings about convergence (Fixes #10173) Dec 16, 2017

@jnothman

This comment has been minimized.

Member

jnothman commented Dec 17, 2017

@jnothman

This comment has been minimized.

Member

jnothman commented Dec 17, 2017

I need to give this a full review

In the meantime, please add an entry under API Changes to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@jotasi

This comment has been minimized.

Contributor

jotasi commented Dec 17, 2017

Certainly! Thanks, I'm looking forward to your comments.
I have added an entry under API Changes. As I changed warnings in multiple modules, I have added one entry under "Misc". Is that alright or should I split it up in entries for every module?

@jnothman

very good work so far!

@@ -311,7 +314,8 @@ def fit(self, X, Y):
y_scores = np.dot(Yk, y_weights) / y_ss
# test for null variance
if np.dot(x_scores.T, x_scores) < np.finfo(np.double).eps:
warnings.warn('X scores are null at iteration %s' % k)
warnings.warn('X scores are null at iteration %s' % k,

This comment has been minimized.

@jnothman

jnothman Dec 17, 2017

Member

It sounds like the warning is in here to note underflow, and I think we should not call this a convergence warning. In particular, we give no straightforward way for the user to improve upon this situation.

This comment has been minimized.

@jotasi

jotasi Dec 18, 2017

Contributor

Ok. I changed it back. I guess the same then applies to the warning about the Y residual here:
https://github.com/jotasi/scikit-learn/blob/14509c2431ae9c683fdd1d36b34d8de4dfbd7b24/sklearn/cross_decomposition/pls_.py#L288-L290
I changed that back as well.

Y = d.target
pls_bynipals = pls_.PLSCanonical(n_components=X.shape[1],
max_iter=2, tol=1e-10)
assert_warns(ConvergenceWarning, pls_bynipals.fit, X, Y)

This comment has been minimized.

@jnothman

jnothman Dec 17, 2017

Member

As there are multiple sources of convergencewarning here, it would be best to use assert_warns_message to check you're triggering the one you expect.

This comment has been minimized.

@jotasi

jotasi Dec 18, 2017

Contributor

As both other ConvergenceWarnings were changed back to UserWarnings (see outdated comment above) this does not apply anymore, does it?

@@ -428,7 +429,8 @@ def _constrained_optimization(self, obj_func, initial_theta, bounds):
fmin_l_bfgs_b(obj_func, initial_theta, bounds=bounds)
if convergence_dict["warnflag"] != 0:
warnings.warn("fmin_l_bfgs_b terminated abnormally with the "
" state: %s" % convergence_dict)
" state: %s" % convergence_dict,

This comment has been minimized.

@jnothman

jnothman Dec 17, 2017

Member

Well this code appears to have test coverage, so it's being triggered somewhere. Do a full test run and check the logs to find out where...?

@jotasi

This comment has been minimized.

Contributor

jotasi commented Dec 18, 2017

Thanks for the review @jnothman. I've made the suggested changes.
Concerning the test of the warning in GaussianProcessRegressor (I assume you ment sklearn/gaussian_process/gpr.py and not sklearn/gaussian_process/gpc.py, as this seems to be the one that is hit according to codecov) I was finally able to somewhat pinpoint which test case under certain circumstances triggers the warning:
The line is hit in test_anisotropic_kernel in sklearn/gaussian_process/test/test_gpr.py but only for the specific environment of test cases 2 & 3 on TravisCI (it is not hit in test case 1). I was not able to reproduce it in my local environment. Using the same environment as Travis in a docker container, I was able to reproduce it, but changing NumPy's and SciPy's versions to above 1.12.1 and 0.18.1 respectively, the warning again disappeared. Even more puzzling, it did not arise in a local virtualenv with the same NumPy and SciPy versions present in the Travis test cases. As this seems to be very fragile, it does not really work as a reliable test case to ensure that the right warning is given.

@jnothman

This comment has been minimized.

Member

jnothman commented Dec 18, 2017

@jotasi

This comment has been minimized.

Contributor

jotasi commented Dec 19, 2017

Perfect. Thanks a lot already to both of you for all your help!

@jnothman

Apart from not repeating .predict, LGTM!

@@ -137,8 +137,10 @@ def test_affinity_propagation_predict_non_convergence():
# At prediction time, consider new samples as noise since there are no
# clusters
to_predict = np.array([[2, 2], [3, 3], [4, 4]])
assert_warns(ConvergenceWarning, af.predict, to_predict)

This comment has been minimized.

@jnothman

jnothman Dec 20, 2017

Member

please just put y = before this line rather than calling predict (and triggering the warning) again

This comment has been minimized.

@jotasi

jotasi Dec 20, 2017

Contributor

I changed that and also added an assert_warns around the call to fit earlier in the same test that (intentionally) triggers a ConvergenceWarning as well (although that one was already there).

@@ -428,7 +429,8 @@ def _constrained_optimization(self, obj_func, initial_theta, bounds):
fmin_l_bfgs_b(obj_func, initial_theta, bounds=bounds)
if convergence_dict["warnflag"] != 0:
warnings.warn("fmin_l_bfgs_b terminated abnormally with the "
" state: %s" % convergence_dict)
" state: %s" % convergence_dict,

This comment has been minimized.

@jnothman

jnothman Dec 20, 2017

Member

The coverage tool seems to think this line is executed by CI. But the number of iterations is the large default, so I'm not sure what kind of data triggers this. I'm therefore okay with leaving it untested, but it's strange that it does seem to be covered somewhere/sometimes in testing, as you note.

@jnothman jnothman changed the title from [MRG] Updated type of warnings about convergence (Fixes #10173) to [MRG+1] Updated type of warnings about convergence (Fixes #10173) Dec 20, 2017

jotasi added some commits Dec 13, 2017

Updated type of warnings about convergence
Some functions still raised UserWarnings for warnings about convergence
problems. These have been updated to raise ConvergenceWarnings instead.
@glemaitre

This comment has been minimized.

Contributor

glemaitre commented Feb 8, 2018

@jotasi Could solve the conflict. Otherwise it looks good. @amueller do you want to have a look?

@jnothman

This comment has been minimized.

Member

jnothman commented Feb 10, 2018

I think we can merge this, @glemaitre, unless you have particular reasons to wait on Andy. I think his review above was basically addressed.

@glemaitre glemaitre merged commit 7700b5a into scikit-learn:master Feb 10, 2018

8 checks passed

ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 94.48%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +5.51% compared to b90661d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@glemaitre

This comment has been minimized.

Contributor

glemaitre commented Feb 10, 2018

Agreed. Thanks @jotasi !!!!

@jotasi jotasi deleted the jotasi:consistently_use_convergence_warnings branch Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment