Fix lbfgsb linesearch out of bound and findAlpha method #633

WeichenXu123 · 2017-03-31T08:07:35Z

What to solve

After a deep check to the LBFGS-B in breeze, I found two serious bug in LBFGS-B:

line search is wrong in this implementation.
LBFGSB.findAlpha method is also wrong.

Fix Line search with bound

According to the LBFGS-B paper
http://users.iems.northwestern.edu/~nocedal/PDFfiles/limited.pdf
The line search in LBFGS-B should be restricted in the bound, but the implementation in breeze do not,
this will cause the optimizer run out of bound, it will cause wrong result or cause line search fail, in some case.
This is the root cause of issue #572, and a series of features is blocking because of this bug:

Huber loss: [SPARK-3181] [ML] Implement RobustRegression with huber loss. apache/spark#14326
Bounded LiR: [SPARK-20029][ML] ML LinearRegression supports bound constrained optimization. apache/spark#17360
Bounded LoR: https://issues.apache.org/jira/browse/SPARK-20047

So that it should be fixed ASAP.
cc @yanboliang

Strong wolfe line search with bound restriction

As mentioned in paper, the best linesearch method for LBFGS-B is strong wolfe line search with bound restriction, this require some modification based on StrongWolfeLineSearch in breeze:

modified strong wolfe condition with bound

We know that strong wolfe condition is to satisfy sufficient decrease condition and curvature condition, BUT according to the paper, the strong wolfe line search in LBFGS-B, the condition should be modified as following:

satisfy sufficient decrease condition
satisfy curvature condition OR new point X(k + 1) hit the bound

Algorithm for strong wolfe condition with bound

Without bound, we already have the following algos (Nodecal & Wright Numerical Optimization p58)

So that, with bound, we can modify this algo into following:

modification in LBFGS-B

In method determineStepSize, first calculate the max step size which won't exceed bound box, then use it as the step bound, call StrongWolfeLineSearch.minimizeWithBound
override takeStep method, in this method check whether the newX exceed bound and correct it. (This is used to avoid numerical error that cause the newX run out of bound)

Fix `findAlpha` method

Unfortunately, the findAlpha method here is also wrong, the wrong implementation cause the subspaceMin point walk out of bound and so that in some case it will also cause the algos crash.
I trace to the code here through several failed testcases with Huber loss.
In summary, there are at least 2 mistakes in this method, let me explain what the method should do first:

find the maximum alpha, satisfiy 0.0 <= alpha <= 1.0, and, for each dimension i, should satisfy:

lowerBound_i - xCauchy_i <= alpha * du_i <= upperBound_i - xCauchy_i

xCauchy is the Cauchy point inside the bound box which will satisfy

lowerBound_i - xCauchy_i <= 0
upperBound_i - xCauchy_i >= 0

and du is the direction vector (details please refer to the paper), the key point is , for each i, the condition above should be satisfied, so that the subspace minimum point computed will be restricted in the bound box.

So that the algo to find the maximum alpha should be:

minimize i: 0 -> dimensionSize  {
  if (du_i < 0) (lowerBound_i - xCauchy_i) / du_i
  else if (du_i > 0) (upperBound_i - xCauchy_i) / du_i
  else Double.Infinity
}

Note that we should handle the case du_i == 0 carefully, otherwise it may generate NaN in computation and will cause the whole algo crash.

Then we can check the implementation in breeze, the logic in findAlpha is wrong. The place where it should use math.min it use math.max. And the code write (ub_i - xc_i) / du_i to be the wrong code ub_i - xc_i / du_i

The second mistake in findAlpha is that it do not handle the case that components of du is zero. This may cause computation run into NaN. We should skip the zero components of du.

Numerical error handling

In theory, the cauchy point, the subspaceMin point, the X point, in the computation, should all be restricted in the bound box. BUT because of floating point error, it may slightly exceed the bound, so I add a adjustWithinBound method to correct it. So that it can avoid the point step out of bound which may cause other bugs.

Test

The following typical algos have been tested:

huber loss regression
bounded LOR
bounded LIR

yanboliang · 2017-03-31T14:57:34Z

math/src/main/scala/breeze/optimize/StrongWolfe.scala

+  def minimize(f: DiffFunction[Double], init: Double = 1.0): Double = {
+    minimizeWithBound(f, init = 1.0, bound = Double.PositiveInfinity)
+  }
+
  /**
   * Performs a line search on the function f, returning a point satisfying
   * the Strong Wolfe conditions. Based on the line search detailed in
   * Nocedal & Wright Numerical Optimization p58.
   */


Should we update the annotation? It looks like out of date.

dlwh · 2017-04-12T23:13:07Z

looks great! thanks so much!

So sorry for the long delay. work and life have been extra busy

dlwh · 2017-04-12T23:12:01Z

math/src/main/scala/breeze/optimize/LBFGSB.scala

-    state.x + (dir *:* stepSize)
+    val newX = state.x + (dir :* stepSize)
+    var i = 0
+    while (i < newX.length) {


i prefer cforRange these days

WeichenXu123 · 2017-04-17T09:45:36Z

cc @dlwh @yanboliang @dbtsai
Thanks!

yanboliang · 2017-04-17T16:04:56Z

I have run tests for bound constraint LiR/LoR and huber regression against this fix, all passed. This looks good to me. Thanks !

dbtsai · 2017-04-17T19:59:51Z

LGTM. Ping @dlwh to cut a new release for our usage in Spark if this looks okay. Thanks.

dlwh · 2017-04-18T04:50:16Z

thanks!

dlwh · 2017-04-18T04:50:26Z

i'll cut a release this week

## What changes were proposed in this pull request? MLlib ```LogisticRegression``` should support bound constrained optimization (only for L2 regularization). Users can add bound constraints to coefficients to make the solver produce solution in the specified range. Under the hood, we call Breeze [```L-BFGS-B```](https://github.com/scalanlp/breeze/blob/master/math/src/main/scala/breeze/optimize/LBFGSB.scala) as the solver for bound constrained optimization. But in the current breeze implementation, there are some bugs in L-BFGS-B, and scalanlp/breeze#633 fixed them. We need to upgrade dependent breeze later, and currently we use the workaround L-BFGS-B in this PR temporary for reviewing. ## How was this patch tested? Unit tests. Author: Yanbo Liang <[email protected]> Closes #17715 from yanboliang/spark-20047. (cherry picked from commit 606432a) Signed-off-by: DB Tsai <[email protected]>

## What changes were proposed in this pull request? MLlib ```LogisticRegression``` should support bound constrained optimization (only for L2 regularization). Users can add bound constraints to coefficients to make the solver produce solution in the specified range. Under the hood, we call Breeze [```L-BFGS-B```](https://github.com/scalanlp/breeze/blob/master/math/src/main/scala/breeze/optimize/LBFGSB.scala) as the solver for bound constrained optimization. But in the current breeze implementation, there are some bugs in L-BFGS-B, and scalanlp/breeze#633 fixed them. We need to upgrade dependent breeze later, and currently we use the workaround L-BFGS-B in this PR temporary for reviewing. ## How was this patch tested? Unit tests. Author: Yanbo Liang <[email protected]> Closes apache#17715 from yanboliang/spark-20047.

ghost · 2017-07-20T17:44:17Z

math/src/main/scala/breeze/optimize/StrongWolfe.scala

@@ -58,12 +58,18 @@ class StrongWolfeLineSearch(maxZoomIter: Int, maxLineSearchIter: Int) extends Cu
  val c1 = 1e-4
  val c2 = 0.9

+  def minimize(f: DiffFunction[Double], init: Double = 1.0): Double = {
+    minimizeWithBound(f, init = 1.0, bound = Double.PositiveInfinity)


LBFGS is passing init value not always equals to 1 to this method. It that right to ignore it here?

In LBFGS-B, we can use 1 as the init value, according to paper, or do you have some better init value ?

Personally, I don't have a better init value, but LBFGS has: https://github.com/scalanlp/breeze/blob/master/math/src/main/scala/breeze/optimize/LBFGS.scala#L76 :)

Let me describe my problem: I've updated spark in my project to latest version (2.2) and some test on logistic regression start failing.
Spark 2.2 uses Breeze 0.13.1 and when I'm explicitly downgrading it to 0.13 tests stop failing.
It seems like in my case regression could not be successfully trained using LBFGS with init value equal to 1.

oh! you're right. we should fix it. like:

def minimize(f: DiffFunction[Double], init: Double = 1.0): Double = { minimizeWithBound(f, init, bound = Double.PositiveInfinity) }

Thanks for finding this bug!

dlwh · 2017-07-20T23:46:02Z

i think it's more that you're ignoring the passed in value of init

…

On Thu, Jul 20, 2017 at 4:35 PM, WeichenXu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In math/src/main/scala/breeze/optimize/StrongWolfe.scala <#633 (comment)>: > @@ -58,12 +58,18 @@ class StrongWolfeLineSearch(maxZoomIter: Int, maxLineSearchIter: Int) extends Cu val c1 = 1e-4 val c2 = 0.9 + def minimize(f: DiffFunction[Double], init: Double = 1.0): Double = { + minimizeWithBound(f, init = 1.0, bound = Double.PositiveInfinity) In LBFGS-B, we can use 1 as the init value, according to paper, or do you have some better init value ? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#633 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAloepiOk2B2kTaPR896BT-RmplS8gFks5sP-RbgaJpZM4MvWUO> .

MLlib ```LogisticRegression``` should support bound constrained optimization (only for L2 regularization). Users can add bound constraints to coefficients to make the solver produce solution in the specified range. Under the hood, we call Breeze [```L-BFGS-B```](https://github.com/scalanlp/breeze/blob/master/math/src/main/scala/breeze/optimize/LBFGSB.scala) as the solver for bound constrained optimization. But in the current breeze implementation, there are some bugs in L-BFGS-B, and scalanlp/breeze#633 fixed them. We need to upgrade dependent breeze later, and currently we use the workaround L-BFGS-B in this PR temporary for reviewing. Unit tests. Author: Yanbo Liang <[email protected]> Closes apache#17715 from yanboliang/spark-20047.

fix lbfgsb linesearch out of bound

7724d71

yanboliang reviewed Mar 31, 2017

View reviewed changes

update

14b54bf

WeichenXu123 force-pushed the fix_lbfgsb_linesearch_out_of_bound branch from f8932dc to 14b54bf Compare April 11, 2017 12:01

dlwh reviewed Apr 12, 2017

View reviewed changes

WeichenXu123 changed the title ~~Fix lbfgsb linesearch out of bound~~ Fix lbfgsb linesearch out of bound and findAlpha method Apr 17, 2017

correct findAlpha method

8f1a5ec

dlwh merged commit dc4e005 into scalanlp:master Apr 18, 2017

WeichenXu123 deleted the fix_lbfgsb_linesearch_out_of_bound branch April 18, 2017 04:51

This was referenced Apr 18, 2017

LBFGS-B return wrong result if we choose bad initial value #572

Closed

[SPARK-20047][ML] Constrained Logistic Regression apache/spark#17715

Closed

ghost reviewed Jul 20, 2017

View reviewed changes

WeichenXu123 mentioned this pull request Jul 21, 2017

Fix strong wolfe line search init value bug #651

Merged

WeichenXu123 mentioned this pull request Aug 7, 2017

[SPARK-3181] [ML] Implement RobustRegression with huber loss. apache/spark#14326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lbfgsb linesearch out of bound and findAlpha method #633

Fix lbfgsb linesearch out of bound and findAlpha method #633

WeichenXu123 commented Mar 31, 2017 •

edited

Loading

yanboliang Mar 31, 2017 •

edited

Loading

dlwh commented Apr 12, 2017

dlwh Apr 12, 2017

WeichenXu123 commented Apr 17, 2017

yanboliang commented Apr 17, 2017 •

edited

Loading

dbtsai commented Apr 17, 2017

dlwh commented Apr 18, 2017

dlwh commented Apr 18, 2017

ghost Jul 20, 2017

WeichenXu123 Jul 20, 2017

ghost Jul 21, 2017

WeichenXu123 Jul 21, 2017

WeichenXu123 Jul 21, 2017

dlwh commented Jul 20, 2017 via email

Fix lbfgsb linesearch out of bound and findAlpha method #633

Fix lbfgsb linesearch out of bound and findAlpha method #633

Conversation

WeichenXu123 commented Mar 31, 2017 • edited Loading

What to solve

Fix Line search with bound

Strong wolfe line search with bound restriction

modified strong wolfe condition with bound

Algorithm for strong wolfe condition with bound

modification in LBFGS-B

Fix findAlpha method

Numerical error handling

Test

yanboliang Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

dlwh commented Apr 12, 2017

dlwh Apr 12, 2017

Choose a reason for hiding this comment

WeichenXu123 commented Apr 17, 2017

yanboliang commented Apr 17, 2017 • edited Loading

dbtsai commented Apr 17, 2017

dlwh commented Apr 18, 2017

dlwh commented Apr 18, 2017

ghost Jul 20, 2017

Choose a reason for hiding this comment

WeichenXu123 Jul 20, 2017

Choose a reason for hiding this comment

ghost Jul 21, 2017

Choose a reason for hiding this comment

WeichenXu123 Jul 21, 2017

Choose a reason for hiding this comment

WeichenXu123 Jul 21, 2017

Choose a reason for hiding this comment

dlwh commented Jul 20, 2017 via email

WeichenXu123 commented Mar 31, 2017 •

edited

Loading

Fix `findAlpha` method

yanboliang Mar 31, 2017 •

edited

Loading

yanboliang commented Apr 17, 2017 •

edited

Loading