You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by facaiy <gi...@git.apache.org> on 2017/04/07 04:28:30 UTC
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
GitHub user facaiy opened a pull request:
https://github.com/apache/spark/pull/17556
[SPARK-16957][MLlib] Use weighted midpoints for split values.
## What changes were proposed in this pull request?
Use weighted midpoints for split values.
## How was this patch tested?
+ [x] add unit test.
+ [x] modify Split's unit test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/facaiy/spark ENH/decision_tree_overflow_and_precision_in_aggregation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17556.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #17556
----
commit 45b74930eea787411855fc35a7ad7198b35d577e
Author: \u989c\u53d1\u624d\uff08Yan Facai\uff09 <fa...@gmail.com>
Date: 2017-04-07T04:02:13Z
TST: add test case
commit c49d3ae7db0e66855b0c896375b11bf51d9ac482
Author: \u989c\u53d1\u624d\uff08Yan Facai\uff09 <fa...@gmail.com>
Date: 2017-04-07T04:05:36Z
ENH: use weighted midpoints
commit 387eb498054289149706ecd2f88593d008fd074f
Author: \u989c\u53d1\u624d\uff08Yan Facai\uff09 <fa...@gmail.com>
Date: 2017-04-07T04:13:44Z
BUG: constant feature, outOfIndex
commit 2e68f1efca59772d1e905474c2392ad0d8b413c8
Author: \u989c\u53d1\u624d\uff08Yan Facai\uff09 <fa...@gmail.com>
Date: 2017-04-07T04:15:09Z
TST: modify split's test case
commit 6a5806f35185596ffda2c88c4879ecaf0be3bda1
Author: \u989c\u53d1\u624d\uff08Yan Facai\uff09 <fa...@gmail.com>
Date: 2017-04-07T04:24:02Z
CLN: move test case
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
I scanned split critical of sklearn and xgboost.
1. sklearn
count all continuous values and split at mean value.
commit 5147fd09c6a063188efde444f47bd006fa5f95f0
sklearn/tree/_splitter.pyx: 484:
```python
current.threshold = (Xf[p - 1] + Xf[p]) / 2.0
```
2. xgboost:
commit 49bdb5c97fccd81b1fdf032eab4599a065c6c4f6
+ If all continuous values are used as candidate, it uses mean value.
src/tree/updater_colmaker.cc: 555:
```c++
e.best.Update(loss_chg, fid, (fvalue + e.last_fvalue) * 0.5f, d_step == -1);
```
+ If continuous feature are quantized, it uses `cut`. I'm not familiar with C++ and update_histmaker.cc is a little complicate, hence I don't know what is `cut` indeed. However, it should be the same with current spark's split critical, I guess.
src/tree/updater_histmaker.cc: 194:
```c++
if (best->Update(static_cast<bst_float>(loss_chg), fid, hist.cut[i], false)) {
```
Anyway, weighted mean is more reasonable than mean or cut value in my option. And the PR is trivial enhancement for tree module, and it's not worth to spend much time because of obvious conclusion.
However, we will be more confident if more feedback of experts are collected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
```
Test Result (1 failure / +1)
org.apache.spark.storage.TopologyAwareBlockReplicationPolicyBehavior.Peers in 2 racks
```
Does anyone know what is this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
is there something wrong with spark CI?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
fix failed case, please retest it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
By the way, it's safe to use mean value as it is match the other libraries. If requested, I'd like to modify the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r111656245
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -104,6 +104,18 @@ class RandomForestSuite extends SparkFunSuite with MLlibTestSparkContext {
assert(splits.distinct.length === splits.length)
}
+ // SPARK-16957: Use weighted midpoints for split values.
+ {
+ val fakeMetadata = new DecisionTreeMetadata(1, 0, 0, 0,
+ Map(), Set(),
+ Array(2), Gini, QuantileStrategy.Sort,
+ 0, 0, 0.0, 0, 0
+ )
+ val featureSamples = Array(0, 1, 0, 0, 1, 0, 1, 1).map(_.toDouble)
+ val splits = RandomForest.findSplitsForContinuousFeature(featureSamples, fakeMetadata, 0)
+ assert(splits === Array(0.5))
--- End diff --
add new case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3673 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3673/testReport)** for PR 17556 at commit [`19eab3a`](https://github.com/apache/spark/commit/19eab3aea2cc15448eb7cac2f08f190fae1e0033).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
The bucketing is trying to to bucket into buckets of equal P(x). It's a condition on P(y | x). That said the right point isn't knowable from the training data, and splitting to balance P(x) on either side of the split within the bucket is perhaps the next-most principled thing to do.
To reach a conclusion though: if we have slightly more net preference for a simple average, we could merge that change for now and decide later to make it weighted.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114493650
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1037,7 +1042,8 @@ 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
+ // perhaps weighted mean will be used later, see SPARK-16957 and Github PR 17556.
--- End diff --
removed. Thanks for your help! @sethah @srowen
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114134468
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ 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
+ // perhaps weighted mean is better in the future, see SPARK-16957 and Github PR 17556.
+ def mean(pre: (Double, Int), cur: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
--- End diff --
Yeah, we should get rid of this method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17556
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r113854473
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -112,9 +138,11 @@ 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 expSplits = Array((1.0 * 2 + 2.0 * 8) / (2 + 8),
+ (2.0 * 8 + 3.0 * 2) / (8 + 2)) // = (1.8, 2.2)
--- End diff --
don't think the comments are necessary. The actual values don't mean much.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114280991
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -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.
+ splitsBuilder += (pre._1 + cur._1) / 2
--- End diff --
Meh, could likewise be one line like above. No big deal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
That's good info. It's a tough call -- matching a known package is always nice. However I agree that a weighted split is a little more theoretically sound (don't have a reference on that though). I'd support this change, myself. It sounds like we won't find an exact match to the R GBM behavior except when each split has equal numbers of classes on either side.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114043568
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -138,9 +169,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 expSplits = Array((1.0 * 1 + 2.0 * 15) / (1 + 15)) // = (1.9375)
--- End diff --
renamed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r113855243
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -138,9 +169,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 expSplits = Array((1.0 * 1 + 2.0 * 15) / (1 + 15)) // = (1.9375)
--- End diff --
just call them `expectedSplits`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
For a (train) sample of continuous series, say {x0, x1, x2, x3, ..., x100}. Now spark select quantile as split point.
Suppose 10-quantiles is used, and x2 is 1st quantile, and x10 is 2nd quantile. It's believed that P(x < x2) ~= P(x2 < x < x10). However, x2 is not perfect. As the data is continuous, there exits one point z is the real point who satisfy P(x < z) == P(z < x < x10).
And it's reasonable that averaged midpoint between x2 and x3 is more appropriate, in my option.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110385244
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ private[spark] object RandomForest extends Logging {
// sort distinct values
val valueCounts = valueCountMap.toSeq.sortBy(_._1).toArray
+ def weightedMean(pre: (Double, Int), cru: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
+ val (curValue, curCount) = cru
+ (preValue * preCount + curValue * curCount) / (preCount + curCount)
--- End diff --
I agree with you. fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110385526
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ private[spark] object RandomForest extends Logging {
// sort distinct values
val valueCounts = valueCountMap.toSeq.sortBy(_._1).toArray
+ def weightedMean(pre: (Double, Int), cru: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
+ val (curValue, curCount) = cru
+ (preValue * preCount + curValue * curCount) / (preCount + curCount)
+ }
+
// 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) {
+ valueCounts
+ .sliding(2)
+ .map{x => weightedMean(x(0), x(1))}
--- End diff --
fixed.
Do you mean use `scanLeft`? It's a little complicate and obscure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use midpoints for split values.
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3682 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3682/testReport)** for PR 17556 at commit [`92df1c8`](https://github.com/apache/spark/commit/92df1c8c58081fc5dbe867249e22d364a8f66e7b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
@srowen Hi, I forget unit tests in python and R. Where can I find document about creating develop environment? thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110374874
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -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) {
--- End diff --
Was this needed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/17556
If we are attempting to match R GBM, it would be great to show, at least on the PR, that we get the same results.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114043439
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -112,9 +138,11 @@ 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 expSplits = Array((1.0 * 2 + 2.0 * 8) / (2 + 8),
+ (2.0 * 8 + 3.0 * 2) / (8 + 2)) // = (1.8, 2.2)
--- End diff --
It may be helpful for code reviewer/maintainer to get the idea clearly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
@sethah what's the issue there ... train/test ought to be from the same distribution, in theory. The empirical distribution of the test data will of course be a little different, but what is the issue with that w.r.t. this change? From a theoretical perspective, picking the midpoint seems more justified than picking an endpoint, and a weighted mean moreso than a midpoint.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3673 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3673/testReport)** for PR 17556 at commit [`19eab3a`](https://github.com/apache/spark/commit/19eab3aea2cc15448eb7cac2f08f190fae1e0033).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114288834
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -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.
+ splitsBuilder += (pre._1 + cur._1) / 2
--- End diff --
Nice! revised.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r111434055
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -104,6 +104,18 @@ class RandomForestSuite extends SparkFunSuite with MLlibTestSparkContext {
assert(splits.distinct.length === splits.length)
}
+ // SPARK-16957: Use weighted midpoints for split values.
+ {
+ val fakeMetadata = new DecisionTreeMetadata(1, 0, 0, 0,
+ Map(), Set(),
+ Array(2), Gini, QuantileStrategy.Sort,
+ 0, 0, 0.0, 0, 0
+ )
+ val featureSamples = Array(0, 1, 0, 0, 1, 0, 1, 1).map(_.toDouble)
+ val splits = RandomForest.findSplitsForContinuousFeature(featureSamples, fakeMetadata, 0)
+ assert(splits === Array(0.5))
--- End diff --
In this block, would you mind adding another test that exercises the `possibleSplits > numSplits` code path? It actually does get called below, but those tests are for other things and I think it's better to make it explicit what we are testing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r113855186
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ 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
+ def weightedMean(pre: (Double, Int), cur: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
+ val (curValue, curCount) = cur
+ (preValue * preCount + curValue * curCount) / (preCount.toDouble + curCount)
+ }
+
val possibleSplits = valueCounts.length - 1
- if (possibleSplits <= numSplits) {
- valueCounts.map(_._1).init
+ if (possibleSplits == 0) {
+ // constant feature
+ Array.empty[Double]
+
--- End diff --
remove this line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
Just a flaky test. Can't be related
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3652 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3652/testReport)** for PR 17556 at commit [`9ca5750`](https://github.com/apache/spark/commit/9ca57505c8211954478a2d54ced48c2561cfb9f9).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114239173
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -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
--- End diff --
@srowen Is it more efficient than sliding?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114112732
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ 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
+ // perhaps weighted mean is better in the future, see SPARK-16957 and Github PR 17556.
+ def mean(pre: (Double, Int), cur: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
--- End diff --
Is it worth factoring a method for this? you could just write `(preValue, _) =` here, but, just dereferncing `._1` isn't so bad, and then, wondering if it saves much to make a method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
@sethah Perhaps it's hard to compare R with Spark's behavior, since many factors involved. I'd like to read R GBM's code, and verify consistency of both side's design on split criteria. Is it OK?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110375319
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ private[spark] object RandomForest extends Logging {
// sort distinct values
val valueCounts = valueCountMap.toSeq.sortBy(_._1).toArray
+ def weightedMean(pre: (Double, Int), cru: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
+ val (curValue, curCount) = cru
+ (preValue * preCount + curValue * curCount) / (preCount + curCount)
+ }
+
// 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) {
+ valueCounts
+ .sliding(2)
+ .map{x => weightedMean(x(0), x(1))}
--- End diff --
Nit: use () instead of {}
There are more efficient ways of writing this but not as compact. I think it's OK unless someone suggests this is performance critical here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3655 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3655/testReport)** for PR 17556 at commit [`9ca5750`](https://github.com/apache/spark/commit/9ca57505c8211954478a2d54ced48c2561cfb9f9).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r113855209
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1037,7 +1051,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)
+
--- End diff --
remove this line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
Hi, I has checked R GBM's code and found that:
R's gbm uses mean value $(x + y) / 2$, not weighted mean $(c_x * x + c_y * y) / (c_x + c_y)$ described in [JIRA SPARK-16957](https://issues.apache.org/jira/browse/SPARK-16957), for split point.
1. code snippet:
[gbm-developers/gbm](https://github.com/gbm-developers/gbm)
commit a1defa382a629f8b97bf9f552dcd821ee7ac9dac
src/node_search.cpp:145:
```c++
else if(cCurrentVarClasses == 0) // variable is continuous
{
// Evaluate the current split
dCurrentSplitValue = 0.5*(dLastXValue + dX);
}
```
2. test
To verify it, I create a toy dataset and take a test on R.
```R
> f = c(0.0, 0.0, 1.0, 1.0, 1.0, 1.0)
> l = c(0, 0, 1, 1, 1, 1)
> df = data.frame(l, f)
> sapply(df, class)
l f
"numeric" "numeric"
> mod <- gbm(l~f, data=df, n.trees=1, bag.fraction=1, n.minobsinnode=1, distribution = "bernoulli")
> pretty.gbm.tree(mod)
SplitVar SplitCodePred LeftNode RightNode MissingNode ErrorReduction Weight
0 0 5.000000e-01 1 2 3 1.333333 6
1 -1 -3.000000e-03 -1 -1 -1 0.000000 2
2 -1 1.500000e-03 -1 -1 -1 0.000000 4
3 -1 1.480297e-19 -1 -1 -1 0.000000 6
Prediction
0 1.480297e-19
1 -3.000000e-03
2 1.500000e-03
3 1.480297e-19
```
As expected,
the root's split point is 5.000000e-01, namely mean value `0.5 = (0 + 1) / 2`, not weighted mean `0.66667 = (0 * 2 + 1 * 4) / 6`.
3. conclusion
I prefer to using weighted mean for split point in the PR, rather than mean value in R's gbm package. How about you? @sethah @srowen
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17556
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
It's looking good, and the R tests pass. I'll also ask @mengxr or maybe @dbtsai if they have any concerns about this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3677 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3677/testReport)** for PR 17556 at commit [`031c61a`](https://github.com/apache/spark/commit/031c61a60d0638dc75133c60c045be2c9204b64b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110385132
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ private[spark] object RandomForest extends Logging {
// sort distinct values
val valueCounts = valueCountMap.toSeq.sortBy(_._1).toArray
+ def weightedMean(pre: (Double, Int), cru: (Double, Int)): Double = {
--- End diff --
fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use midpoints for split values.
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
Merged to master. I'm not against putting it into 2.2, but I'm conscious we already even had an RC
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r111656235
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -112,9 +124,9 @@ 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))
+ assert(splits === Array(1.8, 2.2))
--- End diff --
done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/17556
I don't mind the weighted midpoints. However, if for a continuous feature we find that many points have the exact same value, we are assuming we may find data points in the test set that are close to but not these same values. But since our train data was clustered at these particular values, perhaps it's not a good assumption. I could live with either method, but maybe a slight preference to match the other libraries.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3677 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3677/testReport)** for PR 17556 at commit [`031c61a`](https://github.com/apache/spark/commit/031c61a60d0638dc75133c60c045be2c9204b64b).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3662 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3662/testReport)** for PR 17556 at commit [`b74702a`](https://github.com/apache/spark/commit/b74702afa958fa3552e494cbe77590d9940bf1fb).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110386358
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -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) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114279011
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -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
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/17556
Seems like a reasonable change. Just left some minor comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r111411026
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -126,9 +138,10 @@ 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))
+ assert(splits === Array(2.0625, 3.5))
--- End diff --
ditto
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r111410877
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -112,9 +124,9 @@ 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))
+ assert(splits === Array(1.8, 2.2))
--- End diff --
It's clearer IMO to do:
`assert(splits === Array((2 * 8 + 1 * 2) / (8 + 2), (2 * 8 + 3 * 2) / (8 + 2)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110391698
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ private[spark] object RandomForest extends Logging {
// sort distinct values
val valueCounts = valueCountMap.toSeq.sortBy(_._1).toArray
+ def weightedMean(pre: (Double, Int), cru: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
+ val (curValue, curCount) = cru
+ (preValue * preCount + curValue * curCount) / (preCount + curCount)
+ }
+
// 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) {
+ valueCounts
+ .sliding(2)
+ .map{x => weightedMean(x(0), x(1))}
--- End diff --
No not scanLeft, just manually building the result array and iterating because it's already known ahead of time how big it is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
I have ran all unit test case of MLlib in Python. However, I am not familiar with R, and I don't want waste too many time on deploying R's environment.
Could CI retest the pr? We can check if some unit tests are still broken. thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114238091
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ 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
+ // perhaps weighted mean is better in the future, see SPARK-16957 and Github PR 17556.
+ def mean(pre: (Double, Int), cur: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
--- End diff --
removed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3654 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3654/testReport)** for PR 17556 at commit [`9ca5750`](https://github.com/apache/spark/commit/9ca57505c8211954478a2d54ced48c2561cfb9f9).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114043563
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ 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
+ def weightedMean(pre: (Double, Int), cur: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
+ val (curValue, curCount) = cur
+ (preValue * preCount + curValue * curCount) / (preCount.toDouble + curCount)
+ }
+
val possibleSplits = valueCounts.length - 1
- if (possibleSplits <= numSplits) {
- valueCounts.map(_._1).init
+ if (possibleSplits == 0) {
+ // constant feature
+ Array.empty[Double]
+
--- End diff --
removed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
It seems OK to me but @sethah or @jkbradley might be good as a second set of eyes. It does slightly alter behavior, but, it does seem like something that should work better in general.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use midpoints for split values.
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3682 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3682/testReport)** for PR 17556 at commit [`92df1c8`](https://github.com/apache/spark/commit/92df1c8c58081fc5dbe867249e22d364a8f66e7b).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3654 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3654/testReport)** for PR 17556 at commit [`9ca5750`](https://github.com/apache/spark/commit/9ca57505c8211954478a2d54ced48c2561cfb9f9).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
http://spark.apache.org/docs/latest/building-spark.html
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110374842
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ private[spark] object RandomForest extends Logging {
// sort distinct values
val valueCounts = valueCountMap.toSeq.sortBy(_._1).toArray
+ def weightedMean(pre: (Double, Int), cru: (Double, Int)): Double = {
--- End diff --
Nit: cru -> cur? or current?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114043558
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1037,7 +1051,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)
+
--- End diff --
removed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3655 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3655/testReport)** for PR 17556 at commit [`9ca5750`](https://github.com/apache/spark/commit/9ca57505c8211954478a2d54ced48c2561cfb9f9).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use midpoints for split values.
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on the issue:
https://github.com/apache/spark/pull/17556
Thanks @srowen!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r111656240
--- Diff: mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala ---
@@ -126,9 +138,10 @@ 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))
+ assert(splits === Array(2.0625, 3.5))
--- End diff --
done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17556
**[Test build #3662 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3662/testReport)** for PR 17556 at commit [`b74702a`](https://github.com/apache/spark/commit/b74702afa958fa3552e494cbe77590d9940bf1fb).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use weighted midpoints for s...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r110375204
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1009,10 +1009,24 @@ private[spark] object RandomForest extends Logging {
// sort distinct values
val valueCounts = valueCountMap.toSeq.sortBy(_._1).toArray
+ def weightedMean(pre: (Double, Int), cru: (Double, Int)): Double = {
+ val (preValue, preCount) = pre
+ val (curValue, curCount) = cru
+ (preValue * preCount + curValue * curCount) / (preCount + curCount)
--- End diff --
I'm probably over-thinking this, but do we have a possible overflow issue in the denominator? like if both are near `Int.MaxValue`. One could be converted `.toDouble` just to make sure
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
OK, weight has been removed when calculating.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
many thanks, @srowen
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use weighted midpoints for split va...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/17556
Ah OK I should think about this more first. Say you have a continuous predictor x and binary output y. Say the optimal split is found to be between 0.1 and 0.2, with 1 observation of 0.1 and 99 of 0.2. Right now the algorithm would pick a split value of 0.2; it certainly can't be > 0.2 or < 0.1 but it's highly unlikely that 0.1 or 0.2 are the actual optimal split value.
A weighted mean says the best split is at 0.199, really. It makes sense if you're attempting to make sure that P(0.1 <= x < 0.199) ~= P(0.199 <= x <= 0.2) -- about half the cases in this critical range fall above and below the split. But really the goal is to find x such that P(y=1 | x) is about 0.5. It's not the same thing but it's also not knowable from the training data.
But 0.15 isn't obviously better either. It would mean that, probably, almost all test values in this critical range are classified as positive, not about half.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17556: [SPARK-16957][MLlib] Use midpoints for split valu...
Posted by sethah <gi...@git.apache.org>.
Github user sethah commented on a diff in the pull request:
https://github.com/apache/spark/pull/17556#discussion_r114457816
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/impl/RandomForest.scala ---
@@ -1037,7 +1042,8 @@ 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
+ // perhaps weighted mean will be used later, see SPARK-16957 and Github PR 17556.
--- End diff --
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. Otherwise, no one will follow up on this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17556: [SPARK-16957][MLlib] Use midpoints for split values.
Posted by facaiy <gi...@git.apache.org>.
Github user facaiy commented on the issue:
https://github.com/apache/spark/pull/17556
How about testing the pr, @SparkQA
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org