[MINOR][ML] Increase Bounded MLOR (without regularization) test error tolerance #30587

WeichenXu123 · 2020-12-03T08:28:24Z

What changes were proposed in this pull request?

Improve LogisticRegression test error tolerance

Why are the changes needed?

When we switch BLAS version, some of the tests will fail due to too strict error tolerance in test.

Does this PR introduce any user-facing change?

No

How was this patch tested?

N/A

SparkQA · 2020-12-03T09:08:34Z

Test build #132107 has finished for PR 30587 at commit e63fc10.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-03T09:17:23Z

Test build #132108 has finished for PR 30587 at commit 07b71e2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-03T09:17:42Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36707/

SparkQA · 2020-12-03T09:47:27Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36707/

SparkQA · 2020-12-03T10:00:52Z

Test build #132109 has finished for PR 30587 at commit b3622fa.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-03T10:09:22Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36708/

SparkQA · 2020-12-03T10:40:34Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36708/

SparkQA · 2020-12-03T11:05:43Z

Test build #132117 has finished for PR 30587 at commit a51581d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-03T11:10:13Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36718/

zhengruifeng · 2020-12-03T11:17:22Z

mllib/src/test/scala/org/apache/spark/ml/classification/LogisticRegressionSuite.scala

+          if (math.abs(v1) < 1E-5 || math.abs(v2) < 1E-5) {
+            // If one of v1 and v2 is very close to zero, then compare absTol
+            assert(v1 ~= v2 absTol 1E-2, s"v1 ~= v2 absTol 0.01 faild: v1=${v1}, v2=${v2}")
+          }


nit： lint

zhengruifeng · 2020-12-03T11:17:57Z

mllib/src/test/scala/org/apache/spark/ml/classification/LogisticRegressionSuite.scala

+            assert(v1 ~= v2 absTol 1E-2, s"v1 ~= v2 absTol 0.01 faild: v1=${v1}, v2=${v2}")
+          }
+          else {
+            assert(v1 ~= v2 relTol 1E-2, s"v1 ~= v2 relTol 0.01 faild: v1=${v1}, v2=${v2}")


I guess you can keep previous logic v1 ~= v2 absTol 1E-3 here.

SparkQA · 2020-12-03T11:37:58Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36718/

SparkQA · 2020-12-03T12:59:12Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36728/

SparkQA · 2020-12-03T13:20:41Z

Test build #132127 has finished for PR 30587 at commit ea1c58c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-03T13:30:29Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36728/

srowen

Hm, is this the best fix? a difference of 0.0001 vs 0.0002 could be significant. Do we just need to run LR more iterations instead?

WeichenXu123 · 2020-12-03T15:06:37Z

@srowen

a difference of 0.0001 vs 0.0002 could be significant.

One possibility is that, the result value will converge to 0, but in limited iterations, it may result in some near zero value like such as 1e-4 or 2e-4, @zhengruifeng Could you help check whether increase iteration will help make than converge fast enough to be zero ? Not sure the best threshold to set here for go into the branch of absTol test.

Note: the probably failed test is the multinomial logistic regression with intercept without regularization with bound test, this test require to run Bounded-LBFGS algo (not LBFGS), this algo impl in breeze seems is buggy (sometimes unstable or converge slow), the algo is complex and hard to fix tho.

srowen · 2020-12-03T15:16:49Z

If that's true, then isn't the 'right' expected value 0, and we're looking for a value at most some epsilon?

WeichenXu123 · 2020-12-04T01:08:11Z

If that's true, then isn't the 'right' expected value 0, and we're looking for a value at most some epsilon?

Reasonable. We should set expected value to be 0. Need check test code.

If that's true, then isn't the 'right' expected value 0, and we're looking for a value at most some epsilon?

Resonable. Found some other issue in test code. Let me check.

zhengruifeng · 2020-12-04T01:20:53Z

@srowen @WeichenXu123

I had found that solver BreezeLBFGSB for Huber seems quite unstable, see SPARK-32060 for details.

When data is shuffled or blockified with different blocksize, BreezeLBFGSB output different solutions in my test for Huber.

WeichenXu123 · 2020-12-04T01:33:20Z

The master code use some weird logic to compare:

(col1.asBreeze - col2.asBreeze).toArray.toSeq.sliding(2).foreach {
        case Seq(v1, v2) => assert(v1 ~= v2 absTol 1E-3)
      }

change it to compare left and right directly.

srowen

Maybe this is fine as a band-aid but still feels funky. Can we simply loosen the tolerance if we can't figure it out, or run more iterations etc to improve stability in the test? maybe even different initial conditions.

zhengruifeng · 2020-12-04T01:56:56Z

  /**
   * When no regularization is applied, the multinomial coefficients lack identifiability
   * because we do not use a pivot class. We can add any constant value to the coefficients
   * and get the same likelihood. If fitting under bound constrained optimization, we don't
   * choose the mean centered coefficients like what we do for unbound problems, since they
   * may out of the bounds. We use this function to check whether two coefficients are equivalent.
   */
  def checkCoefficientsEquivalent(coefficients1: Matrix, coefficients2: Matrix): Unit = {
    coefficients1.colIter.zip(coefficients2.colIter).foreach { case (col1: Vector, col2: Vector) =>
      (col1.asBreeze - col2.asBreeze).toArray.toSeq.sliding(2).foreach {
        case Seq(v1, v2) => assert(v1 ~= v2 absTol 1E-3)
      }
    }
  }

According to the comments, it looks that this compare logic is designed on purpose. @WeichenXu123

SparkQA · 2020-12-04T02:14:57Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36770/

SparkQA · 2020-12-04T02:38:05Z

Test build #132169 has finished for PR 30587 at commit 6632ac4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-04T02:40:59Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36770/

SparkQA · 2020-12-04T07:11:28Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36796/

SparkQA · 2020-12-04T07:30:39Z

Test build #132194 has finished for PR 30587 at commit 7af0a12.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-04T07:40:02Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36796/

WeichenXu123 · 2020-12-04T08:44:22Z

@srowen Is this change ok now?
We need backport this to spark 3.1 branch, the adaptive blockify feature added in spark 3.1 increase the probability of this Bounded MLOR (without regularization) test failure.

srowen · 2020-12-04T13:44:42Z

I think it's OK as a band-aid and OK to back port

srowen · 2020-12-08T14:46:48Z

@WeichenXu123 go ahead and merge and backport if you need it in 3.1

… tolerance ### What changes were proposed in this pull request? Improve LogisticRegression test error tolerance ### Why are the changes needed? When we switch BLAS version, some of the tests will fail due to too strict error tolerance in test. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes #30587 from WeichenXu123/fix_lor_test. Authored-by: Weichen Xu <weichen.xu@databricks.com> Signed-off-by: Weichen Xu <weichen.xu@databricks.com> (cherry picked from commit f021f6d) Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

WeichenXu123 · 2020-12-09T03:20:19Z

merge to master and branch-3.1, thanks!

init

e63fc10

github-actions bot added the ML label Dec 3, 2020

WeichenXu123 added 2 commits December 3, 2020 16:35

update

07b71e2

update

b3622fa

update

a51581d

zhengruifeng reviewed Dec 3, 2020

View reviewed changes

update

ea1c58c

srowen reviewed Dec 3, 2020

View reviewed changes

update

6632ac4

srowen reviewed Dec 4, 2020

View reviewed changes

update

7af0a12

WeichenXu123 changed the title ~~[MINOR][ML] Improve LogisticRegression test error tolerance~~ [MINOR][ML] Improve Bounded MLOR (without regularization) test error tolerance Dec 4, 2020

WeichenXu123 changed the title ~~[MINOR][ML] Improve Bounded MLOR (without regularization) test error tolerance~~ [MINOR][ML] Increase Bounded MLOR (without regularization) test error tolerance Dec 4, 2020

WeichenXu123 closed this in f021f6d Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MINOR][ML] Increase Bounded MLOR (without regularization) test error tolerance #30587

[MINOR][ML] Increase Bounded MLOR (without regularization) test error tolerance #30587

WeichenXu123 commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

zhengruifeng Dec 3, 2020

zhengruifeng Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

srowen left a comment

WeichenXu123 commented Dec 3, 2020 •

edited

Loading

srowen commented Dec 3, 2020

WeichenXu123 commented Dec 4, 2020

zhengruifeng commented Dec 4, 2020 •

edited

Loading

WeichenXu123 commented Dec 4, 2020 •

edited

Loading

srowen left a comment

zhengruifeng commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

WeichenXu123 commented Dec 4, 2020

srowen commented Dec 4, 2020

srowen commented Dec 8, 2020

WeichenXu123 commented Dec 9, 2020

[MINOR][ML] Increase Bounded MLOR (without regularization) test error tolerance #30587

[MINOR][ML] Increase Bounded MLOR (without regularization) test error tolerance #30587

Conversation

WeichenXu123 commented Dec 3, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

zhengruifeng Dec 3, 2020

Choose a reason for hiding this comment

zhengruifeng Dec 3, 2020

Choose a reason for hiding this comment

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

srowen left a comment

Choose a reason for hiding this comment

WeichenXu123 commented Dec 3, 2020 • edited Loading

srowen commented Dec 3, 2020

WeichenXu123 commented Dec 4, 2020

zhengruifeng commented Dec 4, 2020 • edited Loading

WeichenXu123 commented Dec 4, 2020 • edited Loading

srowen left a comment

Choose a reason for hiding this comment

zhengruifeng commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

SparkQA commented Dec 4, 2020

WeichenXu123 commented Dec 4, 2020

srowen commented Dec 4, 2020

srowen commented Dec 8, 2020

WeichenXu123 commented Dec 9, 2020

WeichenXu123 commented Dec 3, 2020 •

edited

Loading

zhengruifeng commented Dec 4, 2020 •

edited

Loading

WeichenXu123 commented Dec 4, 2020 •

edited

Loading