Tolerate QN linesearch failures when it's harmless

[achirkin]

In some cases, the linesearch subroutine of the Quasi-Newton solver (logistic regression) may fail in a way that is tolerable for the solver. Currently, any linesearch failure makes the outer algorithm stop, and most of the time the found solution seems to be acceptable to a good precision. This proposal suggests to ignore some non-critical failures ( LS_MAX_ITERS_REACHED and LS_INVALID_STEP_MIN) and stop peacefully if the convergence check triggers or if the function change is too small.

The PR mostly changes the logging behavior, with one small exception. The last condition in update_and_check won't trigger if the linesearch error is not critical and if the target function goes down (i.e. the solver still advances). Previously, the solver would stop independently of the target change.

[achirkin]

Note, this PR makes more sense with #3777 , which sets a reasonable default for param.delta. And it ought to be the last PR to close #3645 (I'll check and close it manually if that's the case).

[cjnolet]

I think the theory behind this approach sounds fine to me- though it might lead to a marginal decrease in perf, it seems like it should provide a better solution in these cases. I just have one little nitpick and a question.

[achirkin]

NB: sometimes, linesearch fails with LS_MAX_ITERS_REACHED while increasing the loss by a small value. Normally, this indicates that the solver has converged, but it could also mean there is a bug in the solver somewhere. In either case we stop the solver, but I am not sure whether we should report that as info, warning, or error.

[dantegd]

@achirkin I was wondering what's the status of this PR? It seems to me that it has addressed @cjnolet's comment, if so is it waiting for a re-review?

[cjnolet]

@dantegd this PR was converted back to a draft after my review to I was under the impression @achirkin was still ironing some things out. @achirkin, please let me know if this is ready and I can re-review.

[achirkin]

Sorry, I should have changed the labels. The solution I propose here is not perfect (it would still report an error when the target seems to grow due to numerical error), and the problem itself is not really pressing (wrong errors in logs). Thus, after a short discussion with @tfeher, we've decided to postpone this until we get a better idea on how to address it.

[achirkin]

Update in the logic:

1. If the lineseach fails, report this as a warning rather than an error.
2. Don't report it at all, if convergence check succeeds.

[tfeher]

Thanks Artem for this PR! Please update the PR description to state under which circumstances the stopping condition could differ from the earlier version. Otherwise it looks good.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@5b36ced). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-21.08    #3791   +/-   ##
===============================================
  Coverage                ?   85.72%           
===============================================
  Files                   ?      230           
  Lines                   ?    18191           
  Branches                ?        0           
===============================================
  Hits                    ?    15595           
  Misses                  ?     2596           
  Partials                ?        0           

Flag	Coverage Δ
dask	48.20% <0.00%> (?)
non-dask	78.16% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b36ced...c9aaab2. Read the comment docs.

[tfeher]

Thanks Artem for the update, the PR looks good to me.

[cjnolet]

LGTM. Thanks Artem!

[cjnolet]

@gpucibot merge