Skip to content
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

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

Closed
wants to merge 7 commits into from

Conversation

WeichenXu123
Copy link
Contributor

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

@github-actions github-actions bot added the ML label Dec 3, 2020
@SparkQA
Copy link

SparkQA commented Dec 3, 2020

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
Copy link

SparkQA commented Dec 3, 2020

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
Copy link

SparkQA commented Dec 3, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2020

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
Copy link

SparkQA commented Dec 3, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2020

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
Copy link

SparkQA commented Dec 3, 2020

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

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}")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: lint

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}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2020

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
Copy link

SparkQA commented Dec 3, 2020

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

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

WeichenXu123 commented Dec 3, 2020

@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
Copy link
Member

srowen commented Dec 3, 2020

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

@WeichenXu123
Copy link
Contributor Author

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
Copy link
Contributor

zhengruifeng commented Dec 4, 2020

@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
Copy link
Contributor Author

WeichenXu123 commented Dec 4, 2020

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.

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

  /**
   * 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
Copy link

SparkQA commented Dec 4, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 4, 2020

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
Copy link

SparkQA commented Dec 4, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 4, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 4, 2020

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
Copy link

SparkQA commented Dec 4, 2020

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

@WeichenXu123
Copy link
Contributor Author

@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.

@WeichenXu123 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 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
@srowen
Copy link
Member

srowen commented Dec 4, 2020

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

@srowen
Copy link
Member

srowen commented Dec 8, 2020

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

WeichenXu123 added a commit that referenced this pull request Dec 9, 2020
… 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
Copy link
Contributor Author

merge to master and branch-3.1, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants