Skip to content

[SPARK-21681][ML] fix bug of MLOR do not work correctly when featureStd contains zero #18896

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.

Sign up for GitHub

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

Closed
WeichenXu123 wants to merge 5 commits into from
Closed

[SPARK-21681][ML] fix bug of MLOR do not work correctly when featureStd contains zero #18896

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3c535e4
init pr
WeichenXu123 Aug 9, 2017
48ea363
add test
WeichenXu123 Aug 15, 2017
8515b20
update testcase description
WeichenXu123 Aug 16, 2017
0f28e5e
update logisticAggregatorSuite
WeichenXu123 Aug 18, 2017
1f4ba14
update testcase
WeichenXu123 Aug 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
update logisticAggregatorSuite
  • Loading branch information
@WeichenXu123
WeichenXu123 committed Aug 21, 2017
commit 0f28e5ea8fb3380d4be8b7771ebc69dd820419bd
13 changes: 11 additions & 2 deletions mllib/src/test/scala/org/apache/spark/ml/optim/aggregator/LogisticAggregatorSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,17 @@ class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
val aggConstantFeature = getNewAggregator(instancesConstantFeature,
Vectors.dense(coefArray ++ interceptArray), fitIntercept = true, isMultinomial = true)
instances.foreach(aggConstantFeature.add)

// constant features should not affect gradient
assert(aggConstantFeature.gradient(0) === 0.0)
def validateGradient(grad: Vector): Unit = {
assert(grad(0) === 0.0)
grad.toArray.foreach { gradientValue =>
Copy link
Contributor

@sethah sethah Aug 19, 2017

Choose a reason for hiding this comment

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

The problem with this test was that it checked that part of the gradient was zero, but didn't check that the rest of the gradient was correct. Here, you're checking that the rest of the gradient isn't nan or infinite, but not that it's actually correct. A more appropriate test, IMO, is to also run an aggregator over the same instances with the constant feature filtered out, then check that the portion of the gradients they share are the same. e.g.

    val aggConstantFeature = getNewAggregator(instancesConstantFeature,
      Vectors.dense(coefArray ++ interceptArray), fitIntercept = true, isMultinomial = true)
    val filteredInstances = instancesConstantFeature.map { case Instance(l, w, f) =>
      Instance(l, w, Vectors.dense(f.toArray.tail))
    }
    val aggMultinomial = getNewAggregator(filteredInstances,
      Vectors.dense(coefArray.slice(3, 6) ++ interceptArray), fitIntercept = true,
      isMultinomial = true)
    filteredInstances.foreach(aggMultinomial.add)
    instancesConstantFeature.foreach(aggConstantFeature.add)

    // constant features should not affect gradient
    assert(aggConstantFeature.gradient.toArray.take(numClasses) === Array.fill(numClasses)(0.0))
    assert(aggMultinomial.gradient.toArray === aggConstantFeature.gradient.toArray.slice(3, 9))

Just to note, this code is just for an example, not meant to be copy and pasted.

assert(!gradientValue.isNaN &&
gradientValue > Double.NegativeInfinity && gradientValue < Double.PositiveInfinity)
}
}

validateGradient(aggConstantFeature.gradient)

val binaryCoefArray = Array(1.0, 2.0)
val intercept = 1.0
Expand All @@ -248,6 +257,6 @@ class LogisticAggregatorSuite extends SparkFunSuite with MLlibTestSparkContext {
isMultinomial = false)
instances.foreach(aggConstantFeatureBinary.add)
// constant features should not affect gradient
assert(aggConstantFeatureBinary.gradient(0) === 0.0)
validateGradient(aggConstantFeatureBinary.gradient)
}
}