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.

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

[SPARK-16957][MLlib] Use midpoints for split values. #17556

Closed
wants to merge 22 commits into from
Closed

[SPARK-16957][MLlib] Use midpoints for split values. #17556

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
45b7493
TST: add test case
facaiy Apr 7, 2017
c49d3ae
ENH: use weighted midpoints
facaiy Apr 7, 2017
387eb49
BUG: constant feature, outOfIndex
facaiy Apr 7, 2017
2e68f1e
TST: modify split's test case
facaiy Apr 7, 2017
6a5806f
CLN: move test case
facaiy Apr 7, 2017
7ad590d
CLN: fix a typo
facaiy Apr 7, 2017
0aaed66
BUG: int, overflow
facaiy Apr 7, 2017
c07ffac
CLN: style mistake, { -> (
facaiy Apr 7, 2017
9ca5750
CLN: mv comment
facaiy Apr 8, 2017
b74702a
TST: revise unit test in python
facaiy Apr 11, 2017
76f4ae8
TST: explicitly show calculation
facaiy Apr 14, 2017
031c61a
TST: add possibleSplits > numSplits
facaiy Apr 15, 2017
1459b14
TST: expSplits -> expectedSplits
facaiy Apr 29, 2017
a094029
CLN: remove blank
facaiy Apr 29, 2017
7c50d4a
ENH: weighted mean -> mean
facaiy Apr 30, 2017
7bb11dd
TST: revise unit test in scala
facaiy Apr 30, 2017
ae0e48e
CLN: mean method is removed
facaiy May 2, 2017
59866fa
CLN: trim whitespace at end of line
facaiy May 2, 2017
10037ea
CLN: refine, possibleSplits <= numSplits
facaiy May 2, 2017
1cae998
CLN: use possibleSplits
facaiy May 2, 2017
92df1c8
CLN: use map to replace for...yield
facaiy May 2, 2017
591d790
CLN: remove comment
facaiy May 3, 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
20 changes: 15 additions & 5 deletions mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,7 +996,7 @@ private[spark] object RandomForest extends Logging {
require(metadata.isContinuous(featureIndex),
"findSplitsForContinuousFeature can only be used to find splits for a continuous feature.")

val splits = if (featureSamples.isEmpty) {
val splits: Array[Double] = if (featureSamples.isEmpty) {
Copy link
Member

Choose a reason for hiding this comment

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

Was this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code block is too long and has 4 exits. Emphasizing its type perhaps is better to be understand, though splits is implied by return type.

Array.empty[Double]
} else {
val numSplits = metadata.numSplits(featureIndex)
Expand All @@ -1009,10 +1009,17 @@ private[spark] object RandomForest extends Logging {
// sort distinct values
val valueCounts = valueCountMap.toSeq.sortBy(_._1).toArray

// if possible splits is not enough or just enough, just return all possible splits
val possibleSplits = valueCounts.length - 1
if (possibleSplits <= numSplits) {
valueCounts.map(_._1).init
if (possibleSplits == 0) {
// constant feature
Array.empty[Double]
} else if (possibleSplits <= numSplits) {
// if possible splits is not enough or just enough, just return all possible splits
val splits = for {
i <- 0 until possibleSplits
} yield (valueCounts(i)._1 + valueCounts(i + 1)._1) / 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen Is it more efficient than sliding?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea. Maybe even just (0 until possibleSplits).map(...).toArray which is probably about the same thing anyway. You might write / 2.0 to be clear it's floating point division


splits.toArray
} else {
// stride between splits
val stride: Double = numSamples.toDouble / (numSplits + 1)
Expand All @@ -1037,7 +1044,10 @@ private[spark] object RandomForest extends Logging {
// makes the gap between currentCount and targetCount smaller,
// previous value is a split threshold.
if (previousGap < currentGap) {
splitsBuilder += valueCounts(index - 1)._1
val pre = valueCounts(index - 1)
val cur = valueCounts(index)
// perhaps weighted mean will be used later, see SPARK-16957 and Github PR 17556.
Copy link
Contributor

@sethah sethah May 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Comments like these tend to just get left around and sit there forever. Unless we file a new JIRA that intends to decide on future behavior, I would like to remove this comment altogether. I'd prefer to just remove it and not create a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed. Thanks for your help! @sethah @srowen

splitsBuilder += (pre._1 + cur._1) / 2
Copy link
Member

Choose a reason for hiding this comment

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

Meh, could likewise be one line like above. No big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! revised.

targetCount += stride
}
index += 1
Expand Down
41 changes: 35 additions & 6 deletions mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,31 @@ class RandomForestSuite extends SparkFunSuite with MLlibTestSparkContext {
assert(splits.distinct.length === splits.length)
}

// SPARK-16957: Use midpoints for split values.
{
val fakeMetadata = new DecisionTreeMetadata(1, 0, 0, 0,
Map(), Set(),
Array(3), Gini, QuantileStrategy.Sort,
0, 0, 0.0, 0, 0
)

// possibleSplits <= numSplits
{
val featureSamples = Array(0, 1, 0, 0, 1, 0, 1, 1).map(_.toDouble)
val splits = RandomForest.findSplitsForContinuousFeature(featureSamples, fakeMetadata, 0)
val expectedSplits = Array((0.0 + 1.0) / 2)
assert(splits === expectedSplits)
}

// possibleSplits > numSplits
{
val featureSamples = Array(0, 0, 1, 1, 2, 2, 3, 3).map(_.toDouble)
val splits = RandomForest.findSplitsForContinuousFeature(featureSamples, fakeMetadata, 0)
val expectedSplits = Array((0.0 + 1.0) / 2, (2.0 + 3.0) / 2)
assert(splits === expectedSplits)
}
}

// find splits should not return identical splits
// when there are not enough split candidates, reduce the number of splits in metadata
{
Expand All @@ -112,9 +137,10 @@ class RandomForestSuite extends SparkFunSuite with MLlibTestSparkContext {
Array(5), Gini, QuantileStrategy.Sort,
0, 0, 0.0, 0, 0
)
val featureSamples = Array(1, 1, 2, 2, 2, 2, 2, 2, 2, 2, 3).map(_.toDouble)
val featureSamples = Array(1, 1, 2, 2, 2, 2, 2, 2, 2, 2, 3, 3).map(_.toDouble)
val splits = RandomForest.findSplitsForContinuousFeature(featureSamples, fakeMetadata, 0)
assert(splits === Array(1.0, 2.0))
val expectedSplits = Array((1.0 + 2.0) / 2, (2.0 + 3.0) / 2)
assert(splits === expectedSplits)
// check returned splits are distinct
assert(splits.distinct.length === splits.length)
}
Expand All @@ -126,9 +152,11 @@ class RandomForestSuite extends SparkFunSuite with MLlibTestSparkContext {
Array(3), Gini, QuantileStrategy.Sort,
0, 0, 0.0, 0, 0
)
val featureSamples = Array(2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 4, 5).map(_.toDouble)
val featureSamples = Array(2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 4, 5)
.map(_.toDouble)
val splits = RandomForest.findSplitsForContinuousFeature(featureSamples, fakeMetadata, 0)
assert(splits === Array(2.0, 3.0))
val expectedSplits = Array((2.0 + 3.0) / 2, (3.0 + 4.0) / 2)
assert(splits === expectedSplits)
}

// find splits when most samples close to the maximum
Expand All @@ -138,9 +166,10 @@ class RandomForestSuite extends SparkFunSuite with MLlibTestSparkContext {
Array(2), Gini, QuantileStrategy.Sort,
0, 0, 0.0, 0, 0
)
val featureSamples = Array(0, 1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2).map(_.toDouble)
val featureSamples = Array(0, 1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2).map(_.toDouble)
val splits = RandomForest.findSplitsForContinuousFeature(featureSamples, fakeMetadata, 0)
assert(splits === Array(1.0))
val expectedSplits = Array((1.0 + 2.0) / 2)
assert(splits === expectedSplits)
}

// find splits for constant feature
Expand Down
12 changes: 6 additions & 6 deletions python/pyspark/mllib/tree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,9 @@ def trainClassifier(cls, data, numClasses, categoricalFeaturesInfo,

>>> print(model.toDebugString())
DecisionTreeModel classifier of depth 1 with 3 nodes
If (feature 0 <= 0.0)
If (feature 0 <= 0.5)
Predict: 0.0
Else (feature 0 > 0.0)
Else (feature 0 > 0.5)
Predict: 1.0
<BLANKLINE>
>>> model.predict(array([1.0]))
Expand Down Expand Up @@ -383,14 +383,14 @@ def trainClassifier(cls, data, numClasses, categoricalFeaturesInfo, numTrees,
Tree 0:
Predict: 1.0
Tree 1:
If (feature 0 <= 1.0)
If (feature 0 <= 1.5)
Predict: 0.0
Else (feature 0 > 1.0)
Else (feature 0 > 1.5)
Predict: 1.0
Tree 2:
If (feature 0 <= 1.0)
If (feature 0 <= 1.5)
Predict: 0.0
Else (feature 0 > 1.0)
Else (feature 0 > 1.5)
Predict: 1.0
<BLANKLINE>
>>> model.predict([2.0])
Expand Down