[SPARK-21818][ML][MLLIB] Fix bug of MultivariateOnlineSummarizer.variance generate negative result #19029

WeichenXu123 · 2017-08-23T10:58:03Z

What changes were proposed in this pull request?

Because of numerical error, MultivariateOnlineSummarizer.variance is possible to generate negative variance.

This is a serious bug because many algos in MLLib
use stddev computed from sqrt(variance)
it will generate NaN and crash the whole algorithm.

we can reproduce this bug use the following code:

    val summarizer1 = (new MultivariateOnlineSummarizer)
      .add(Vectors.dense(3.0), 0.7)
    val summarizer2 = (new MultivariateOnlineSummarizer)
      .add(Vectors.dense(3.0), 0.4)
    val summarizer3 = (new MultivariateOnlineSummarizer)
      .add(Vectors.dense(3.0), 0.5)
    val summarizer4 = (new MultivariateOnlineSummarizer)
      .add(Vectors.dense(3.0), 0.4)

    val summarizer = summarizer1
      .merge(summarizer2)
      .merge(summarizer3)
      .merge(summarizer4)

    println(summarizer.variance(0))

This PR fix the bugs in mllib.stat.MultivariateOnlineSummarizer.variance and ml.stat.SummarizerBuffer.variance, and several places in WeightedLeastSquares

How was this patch tested?

test cases added.

srowen · 2017-08-23T11:00:34Z

mllib/src/main/scala/org/apache/spark/ml/stat/Summarizer.scala

          realVariance(i) = (currM2n(i) + deltaMean(i) * deltaMean(i) * weightSum(i) *
            (totalWeightSum - weightSum(i)) / totalWeightSum) / denominator
+          // Because of numerical error, it is possible to get negative real variance
+          if (realVariance(i) < 0.0) {


Just use math.max(0.0 ...) in the line above? no need to assign it twice.

The computation of variance may be touch this numerical error, it seems WeightedLeastSquares also use the same method to compute variance , does it will have similar issue? @WeichenXu123

Hmm.. WeightedLeastSquares use another way to compute variance Var(X) = E(X^2) - E(X)^2. But it seems also possible to have this problem.

SparkQA · 2017-08-23T12:05:06Z

Test build #81032 has finished for PR 19029 at commit 9c92730.

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

SparkQA · 2017-08-25T06:51:10Z

Test build #81118 has finished for PR 19029 at commit c24292c.

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

yanboliang

One minor comments, otherwise, LGTM.

yanboliang · 2017-08-25T09:12:35Z

mllib/src/main/scala/org/apache/spark/ml/optim/WeightedLeastSquares.scala

     * Weighted population standard deviation of labels.
     */
-    def bStd: Double = math.sqrt(bbSum / wSum - bBar * bBar)
+    def bStd: Double = math.sqrt(math.max(bbSum / wSum - bBar * bBar, 0.0))


Please add comment here and bellow to clarify that we are preventing from negative value caused by numerical error.

SparkQA · 2017-08-25T10:36:23Z

Test build #81127 has finished for PR 19029 at commit 9a47579.

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

SparkQA · 2017-08-25T10:44:05Z

Test build #81129 has finished for PR 19029 at commit 56c0d41.

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

srowen · 2017-08-26T08:45:07Z

mllib/src/main/scala/org/apache/spark/ml/optim/WeightedLeastSquares.scala

+     * We prevent variance from negative value caused by numerical error.
     */
-    def bStd: Double = math.sqrt(bbSum / wSum - bBar * bBar)
+    def bStd: Double = math.sqrt(math.max(bbSum / wSum - bBar * bBar, 0.0))


There are a couple more places where variance is computed in this file -- I think they need this too?

SparkQA · 2017-08-27T11:35:47Z

Test build #81167 has finished for PR 19029 at commit 21e7ff7.

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

srowen · 2017-08-27T13:10:02Z

mllib/src/main/scala/org/apache/spark/ml/optim/WeightedLeastSquares.scala


    /**
     * Weighted population standard deviation of labels.
+     * We prevent variance from negative value caused by numerical error.


I'm not so against this, but this is really an implementation detail and not relevant to the caller. It's a value that is by definition nonnegative.

SparkQA · 2017-08-28T02:45:44Z

Test build #81171 has finished for PR 19029 at commit c40eba3.

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

srowen · 2017-08-28T06:42:07Z

Merged to master/2.2

…ance generate negative result Because of numerical error, MultivariateOnlineSummarizer.variance is possible to generate negative variance. **This is a serious bug because many algos in MLLib** **use stddev computed from** `sqrt(variance)` **it will generate NaN and crash the whole algorithm.** we can reproduce this bug use the following code: ``` val summarizer1 = (new MultivariateOnlineSummarizer) .add(Vectors.dense(3.0), 0.7) val summarizer2 = (new MultivariateOnlineSummarizer) .add(Vectors.dense(3.0), 0.4) val summarizer3 = (new MultivariateOnlineSummarizer) .add(Vectors.dense(3.0), 0.5) val summarizer4 = (new MultivariateOnlineSummarizer) .add(Vectors.dense(3.0), 0.4) val summarizer = summarizer1 .merge(summarizer2) .merge(summarizer3) .merge(summarizer4) println(summarizer.variance(0)) ``` This PR fix the bugs in `mllib.stat.MultivariateOnlineSummarizer.variance` and `ml.stat.SummarizerBuffer.variance`, and several places in `WeightedLeastSquares` test cases added. Author: WeichenXu <[email protected]> Closes #19029 from WeichenXu123/fix_summarizer_var_bug. (cherry picked from commit 0456b40) Signed-off-by: Sean Owen <[email protected]>

…ance generate negative result Because of numerical error, MultivariateOnlineSummarizer.variance is possible to generate negative variance. **This is a serious bug because many algos in MLLib** **use stddev computed from** `sqrt(variance)` **it will generate NaN and crash the whole algorithm.** we can reproduce this bug use the following code: ``` val summarizer1 = (new MultivariateOnlineSummarizer) .add(Vectors.dense(3.0), 0.7) val summarizer2 = (new MultivariateOnlineSummarizer) .add(Vectors.dense(3.0), 0.4) val summarizer3 = (new MultivariateOnlineSummarizer) .add(Vectors.dense(3.0), 0.5) val summarizer4 = (new MultivariateOnlineSummarizer) .add(Vectors.dense(3.0), 0.4) val summarizer = summarizer1 .merge(summarizer2) .merge(summarizer3) .merge(summarizer4) println(summarizer.variance(0)) ``` This PR fix the bugs in `mllib.stat.MultivariateOnlineSummarizer.variance` and `ml.stat.SummarizerBuffer.variance`, and several places in `WeightedLeastSquares` test cases added. Author: WeichenXu <[email protected]> Closes apache#19029 from WeichenXu123/fix_summarizer_var_bug. (cherry picked from commit 0456b40) Signed-off-by: Sean Owen <[email protected]>

init pr

9c92730

srowen reviewed Aug 23, 2017

View reviewed changes

update

c24292c

srowen approved these changes Aug 25, 2017

View reviewed changes

yanboliang reviewed Aug 25, 2017

View reviewed changes

WeichenXu123 added 2 commits August 25, 2017 17:20

add comment

9a47579

update comment

56c0d41

srowen requested changes Aug 26, 2017

View reviewed changes

update

21e7ff7

srowen reviewed Aug 27, 2017

View reviewed changes

update

c40eba3

asfgit closed this in 0456b40 Aug 28, 2017

WeichenXu123 deleted the fix_summarizer_var_bug branch August 28, 2017 10:28

[SPARK-21818][ML][MLLIB] Fix bug of MultivariateOnlineSummarizer.variance generate negative result #19029

[SPARK-21818][ML][MLLIB] Fix bug of MultivariateOnlineSummarizer.variance generate negative result #19029

Uh oh!

Conversation

WeichenXu123 commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

yanboliang Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 23, 2017

Uh oh!

SparkQA commented Aug 25, 2017

Uh oh!

yanboliang left a comment

Choose a reason for hiding this comment

Uh oh!

yanboliang Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 25, 2017

Uh oh!

SparkQA commented Aug 25, 2017

Uh oh!

srowen Aug 26, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 27, 2017

Uh oh!

srowen Aug 27, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 28, 2017

Uh oh!

srowen commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

WeichenXu123 commented Aug 23, 2017 •

edited

Loading

srowen commented Aug 28, 2017 •

edited

Loading