You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by chouqin <gi...@git.apache.org> on 2014/09/09 11:03:15 UTC
[GitHub] spark pull request: [SPARK-2207][SPARK-3272]Add minimum informatio...
GitHub user chouqin opened a pull request:
https://github.com/apache/spark/pull/2332
[SPARK-2207][SPARK-3272]Add minimum information gain and minimum instances per node as training parameters for decision tree.
These two parameters can act as early stop rules to do pre-pruning. When a split cause cause left or right child to have less than `minInstancesPerNode` or has less information gain than `minInfoGain`, current node will not be split by this split.
When there is no possible splits that satisfy requirements, there is no useful information gain stats, but we still need to calculate the predict value for current node. So I separated calculation of predict from calculation of information gain, which can also save computation when the number of possible splits is large. Please see [SPARK-3272](https://issues.apache.org/jira/browse/SPARK-3272) for more details.
CC: @mengxr @manishamde @jkbradley, please help me review this, thanks.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chouqin/spark dt-preprune
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2332.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 #2332
----
commit ac4237808090237fe4c562da8c88c55c330d451f
Author: qiping.lqp <qi...@alibaba-inc.com>
Date: 2014-09-09T03:17:58Z
add min info gain and min instances per node parameters in decision tree
commit ff34845c8e43f5b9755dd1fdf428be8b2284c68b
Author: qiping.lqp <qi...@alibaba-inc.com>
Date: 2014-09-09T04:29:12Z
separate calculation of predict of node from calculation of info gain
commit 987cbf4b177f29e232bf2ba2ca595ea7015694da
Author: qiping.lqp <qi...@alibaba-inc.com>
Date: 2014-09-09T04:30:01Z
fix bug
commit f195e830a94097e5d6d42f22c67c32ca8900d848
Author: qiping.lqp <qi...@alibaba-inc.com>
Date: 2014-09-09T06:04:20Z
fix style
commit 845c6fa58c00bfba426e56e71eb46a6f8c3f5985
Author: qiping.lqp <qi...@alibaba-inc.com>
Date: 2014-09-09T06:05:37Z
fix style
commit e72c7e4d0ad015fdf25ea2959bdbf524056e38ca
Author: qiping.lqp <qi...@alibaba-inc.com>
Date: 2014-09-09T06:52:24Z
add comments
commit 46b891fd7f30b9f2d439134931b35dab387fe2b1
Author: qiping.lqp <qi...@alibaba-inc.com>
Date: 2014-09-09T08:09:34Z
fix bug
commit cadd569cf64d6eb7b9c9979a5066a2f63f15fed9
Author: qiping.lqp <qi...@alibaba-inc.com>
Date: 2014-09-09T08:48:51Z
add api docs
----
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17321579
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/model/Split.scala ---
@@ -66,3 +68,11 @@ private[tree] class DummyHighSplit(feature: Int, featureType: FeatureType)
private[tree] class DummyCategoricalSplit(feature: Int, featureType: FeatureType)
extends Split(feature, Double.MaxValue, featureType, List())
+private[tree] object Split {
+ /**
+ * A [[org.apache.spark.mllib.tree.model.Split]] object to denote that
+ * we can't find a valid split that satisfies minimum info gain
+ * or minimum number of instances per node.
+ */
+ val noSplit = new Split(-1, Double.MinValue, FeatureType.Continuous, List())
--- End diff --
noSplit is never really used, so it could be removed. I would treat splits as normal (returning a split even if its gain is invalid); this should work since the split is never checked against noSplit (since the gain has all needed information).
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17334675
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/configuration/Strategy.scala ---
@@ -49,6 +49,13 @@ import org.apache.spark.mllib.tree.configuration.QuantileStrategy._
* k) implies the feature n is categorical with k categories 0,
* 1, 2, ... , k-1. It's important to note that features are
* zero-indexed.
+ * @param minInstancesPerNode Minimum number of nodes each child must have after split.
--- End diff --
yes, I will change 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 pull request: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55016922
Jenkins, add to whitelist.
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55016955
this is ok to test
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55019741
@chouqin Thanks for this update---it will be great to have these 2 options supported. My comments are mostly about simplifying the code: removing Predict and Split.noSplit, and the related simplifications. We will not save much computation by avoiding calculating predictions. However, we will definitely save a lot of computation by supporting these 2 options to allow early stopping on some datasets!
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17321566
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/configuration/Strategy.scala ---
@@ -49,6 +49,13 @@ import org.apache.spark.mllib.tree.configuration.QuantileStrategy._
* k) implies the feature n is categorical with k categories 0,
* 1, 2, ... , k-1. It's important to note that features are
* zero-indexed.
+ * @param minInstancesPerNode Minimum number of nodes each child must have after split.
--- End diff --
"number of nodes" --> "number of instances"
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55069323
@chouqin Thanks for the updates! This looks basically ready, except for the edge cases in the test suite. I tested it and it ran fine. I think those complaints about public classes are unrelated. Once the test suite is updated, I'd say it is ready.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55189419
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20116/consoleFull) for PR 2332 at commit [`f1d11d1`](https://github.com/apache/spark/commit/f1d11d15fe519f9ef9d4e1158b309dc6af38864e).
* This patch **passes** 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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55057697
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20058/consoleFull) for PR 2332 at commit [`d593ec7`](https://github.com/apache/spark/commit/d593ec70d70b633b72e260c38e89d87ab14fcd69).
* This patch merges cleanly.
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55053558
@jkbradley thanks for your comments, I will change my code accordingly. As for the Predict class, I still think it is needed, for the following reasons:
1. Saving of computation, for each split, it will traverse a array of bins two times(one to add, one to find the index that has the maximum value), I don't think this saving is trival.
2. As for code simplicity, I think predict value for a node should not tied to information gain for a split(information gain ). I have read Weka and scikit-learn's decision tree code, they don't store a predict value along with split's information gain stats. I think the changed code may be easy to understand somehow.
3. For the early return of calculate `calculateGainForSplit`, when left child or right child has less than `minInstancesPerNode` we can just return an invalid information gain stats, without calculate the predict value.If all splits are early returned, we need a way to calculate the predict value for current node.
If you don't like creating a new `Predict` class, I can use a tuple to replace that, but this seems to be harder to understand.
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55032991
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20051/consoleFull) for PR 2332 at commit [`efcc736`](https://github.com/apache/spark/commit/efcc7369f7f52de2810446c6fb976ab1743a63cf).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `class Predict(`
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55177933
@chouqin Thanks for the updates! LGTM
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55064961
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20069/consoleFull) for PR 2332 at commit [`0278a11`](https://github.com/apache/spark/commit/0278a1198017aae578be3109a8311abc1f9a8e14).
* This patch merges cleanly.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17386996
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/DecisionTree.scala ---
@@ -898,6 +928,10 @@ object DecisionTree extends Serializable with Logging {
(bestFeatureSplit, bestFeatureGainStats)
}
}.maxBy(_._2.gain)
+
+ require(predict.isDefined, "must calculate predict for each node")
--- End diff --
nit: Use `assert` instead of `require`. The latter throws `IllegalArgumentException`, which doesn't apply here. (not necessary to update)
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17321558
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/DecisionTree.scala ---
@@ -130,7 +130,7 @@ class DecisionTree (private val strategy: Strategy) extends Serializable with Lo
// Find best split for all nodes at a level.
timer.start("findBestSplits")
- val splitsStatsForLevel: Array[(Split, InformationGainStats)] =
+ val splitsStatsForLevel: Array[(Split, InformationGainStats, Predict)] =
--- End diff --
See comment for Predict.scala: If prediction info is stored in InformationGainStats, these several lines will not need to be changed.
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-54947043
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 pull request: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55054642
@chouqin Could you also please add tag [mllib] to the PR title?
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17386953
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/DecisionTree.scala ---
@@ -130,7 +130,7 @@ class DecisionTree (private val strategy: Strategy) extends Serializable with Lo
// Find best split for all nodes at a level.
timer.start("findBestSplits")
- val splitsStatsForLevel: Array[(Split, InformationGainStats)] =
+ val splitsStatsForLevel: Array[(Split, InformationGainStats, Predict)] =
--- End diff --
(Ignore comment since we are keeping Predict)
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17321563
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/DecisionTree.scala ---
@@ -803,11 +830,16 @@ object DecisionTree extends Serializable with Logging {
val leftChildStats = binAggregates.getImpurityCalculator(nodeFeatureOffset, splitIdx)
val rightChildStats = binAggregates.getImpurityCalculator(nodeFeatureOffset, numSplits)
rightChildStats.subtract(leftChildStats)
+ predict = Some(predict.getOrElse(calculatePredict(leftChildStats, rightChildStats)))
val gainStats =
calculateGainForSplit(leftChildStats, rightChildStats, nodeImpurity, level, metadata)
(splitIdx, gainStats)
}.maxBy(_._2.gain)
- (splits(featureIndex)(bestFeatureSplitIndex), bestFeatureGainStats)
+ if (bestFeatureGainStats == InformationGainStats.invalidInformationGainStats) {
--- End diff --
I think you could avoid explicitly checking for invalidInformationGainStats since the gain is Double.minValue. At the very end of the maxBy calls, you could then check to see if the information gain is Double.minValue, in which case we know that no split is worth doing. That should simplify the code here and in the other maxBy calls below.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17355688
--- Diff: mllib/src/test/scala/org/apache/spark/mllib/tree/DecisionTreeSuite.scala ---
@@ -674,6 +676,45 @@ class DecisionTreeSuite extends FunSuite with LocalSparkContext {
validateClassifier(model, arr, 0.6)
}
+ test("split must satisfy min instances per node requirements") {
--- End diff --
minInstancesPerNode's default value is 1, which is tested by other test cases.
If I change `strategy.minInstancesPerNode` to 2(which is 4 right now), other parts don't need to change, because no there are only three instances, no split can make left and rightchild to have 2 instances. So I think setting `minInstancesPerNode` to 2 is more reasonable than 4 in this test case. Does this change satisfy your needs? Or should I increase the instances number and test `minInstancesPerNode` is 2 and 4 at the same time?
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55179015
@chouqin I made very minor inline comments. It is not necessary to update the PR. I'm going to merge this if Jenkins is happy, and @jkbradley will make those changes in his following 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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by manishamde <gi...@git.apache.org>.
Github user manishamde commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-54947658
@chouqin Thanks for the PR. I won't be able to comment since I am on a break now. @jkbradley and @mengxr reviews should be sufficient. :-)
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55068261
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20069/consoleFull) for PR 2332 at commit [`0278a11`](https://github.com/apache/spark/commit/0278a1198017aae578be3109a8311abc1f9a8e14).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression] `
* `case class LastFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction `
* `case class Abs(child: Expression) extends UnaryExpression `
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17321574
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/model/Predict.scala ---
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.mllib.tree.model
+
+import org.apache.spark.annotation.DeveloperApi
+
+/**
+ * :: DeveloperApi ::
+ * Predicted value for a node
+ * @param predict predicted value
+ * @param prob probability of the label (classification only)
+ */
+@DeveloperApi
+class Predict(
--- End diff --
As I said in the other PR which included a Predict class, I really do not think that an entire class for holding predictions is needed or helpful. The InformationGainStats can hold the prediction information, even if the InformationGainStats.gain value is set to Double.MinValue (for an invalid split).
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55023385
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20051/consoleFull) for PR 2332 at commit [`efcc736`](https://github.com/apache/spark/commit/efcc7369f7f52de2810446c6fb976ab1743a63cf).
* This patch merges cleanly.
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17334865
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/configuration/Strategy.scala ---
@@ -61,6 +68,8 @@ class Strategy (
val maxBins: Int = 32,
val quantileCalculationStrategy: QuantileStrategy = Sort,
val categoricalFeaturesInfo: Map[Int, Int] = Map[Int, Int](),
+ val minInstancesPerNode: Int = 0,
--- End diff --
1 seems more reasonable than 0, when a spit can't split a node apart(either left or right child has 0 instances), this split should not be considered.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17387005
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/model/Predict.scala ---
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.mllib.tree.model
+
+import org.apache.spark.annotation.DeveloperApi
+
+/**
+ * :: DeveloperApi ::
+ * Predicted value for a node
+ * @param predict predicted value
+ * @param prob probability of the label (classification only)
+ */
+@DeveloperApi
--- End diff --
If this is a package private class, it is not necessary to mark `DeveloperApi`.
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17321560
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/DecisionTree.scala ---
@@ -764,7 +764,31 @@ object DecisionTree extends Serializable with Logging {
val gain = impurity - leftWeight * leftImpurity - rightWeight * rightImpurity
- new InformationGainStats(gain, impurity, leftImpurity, rightImpurity, predict, prob)
+ // if information gain doesn't satisfy minimum information gain,
+ // then this split is invalid, return invalid information gain stats.
+ if (gain < metadata.minInfoGain) {
+ return InformationGainStats.invalidInformationGainStats
+ }
+
+ new InformationGainStats(gain, impurity, leftImpurity, rightImpurity)
+ }
+
+ /**
+ * Calculate predict value for current node, given stats of any split.
+ * Note that this function is called only once for each node.
+ * @param leftImpurityCalculator left node aggregates for a split
+ * @param rightImpurityCalculator right node aggregates for a node
+ * @return predict value for current node
+ */
+ private def calculatePredict(
--- End diff --
See comment for Predict.scala: If prediction info is stored in InformationGainStats, this calculation can remain in calculateGainForSplit(). It is really not very expensive to compute the prediction, so I think we gain a lot in simplicity and lose very little in computational cost by keeping everything in InformationGainStats.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55126783
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20105/consoleFull) for PR 2332 at commit [`f1d11d1`](https://github.com/apache/spark/commit/f1d11d15fe519f9ef9d4e1158b309dc6af38864e).
* This patch merges cleanly.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55063959
@jkbradley I have removed `noSplit` object and add `private[tree]` to `Predict`.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17367203
--- Diff: mllib/src/test/scala/org/apache/spark/mllib/tree/DecisionTreeSuite.scala ---
@@ -674,6 +676,45 @@ class DecisionTreeSuite extends FunSuite with LocalSparkContext {
validateClassifier(model, arr, 0.6)
}
+ test("split must satisfy min instances per node requirements") {
--- End diff --
Sorry I missed this comment before; 2 sounds good.
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17321571
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/configuration/Strategy.scala ---
@@ -61,6 +68,8 @@ class Strategy (
val maxBins: Int = 32,
val quantileCalculationStrategy: QuantileStrategy = Sort,
val categoricalFeaturesInfo: Map[Int, Int] = Map[Int, Int](),
+ val minInstancesPerNode: Int = 0,
--- End diff --
Perhaps default should be 1 so that predict() is well-defined. I would actually require >= 1.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17338800
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/model/Split.scala ---
@@ -66,3 +68,11 @@ private[tree] class DummyHighSplit(feature: Int, featureType: FeatureType)
private[tree] class DummyCategoricalSplit(feature: Int, featureType: FeatureType)
extends Split(feature, Double.MaxValue, featureType, List())
+private[tree] object Split {
+ /**
+ * A [[org.apache.spark.mllib.tree.model.Split]] object to denote that
+ * we can't find a valid split that satisfies minimum info gain
+ * or minimum number of instances per node.
+ */
+ val noSplit = new Split(-1, Double.MinValue, FeatureType.Continuous, List())
--- End diff --
yes, you are right, noSplit is not needed, I wil remove this, 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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55061865
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20058/consoleFull) for PR 2332 at commit [`d593ec7`](https://github.com/apache/spark/commit/d593ec70d70b633b72e260c38e89d87ab14fcd69).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `class Predict(`
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17387008
--- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/model/Predict.scala ---
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.mllib.tree.model
+
+import org.apache.spark.annotation.DeveloperApi
+
+/**
+ * :: DeveloperApi ::
+ * Predicted value for a node
+ * @param predict predicted value
+ * @param prob probability of the label (classification only)
+ */
+@DeveloperApi
+private[tree] class Predict(
+ val predict: Double,
+ val prob: Double = 0.0) extends Serializable{
--- End diff --
space before `{`
---
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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55054562
@chouqin Thanks for your responses. I think you've convinced me that Predict is reasonable, since it is a different concept from info gain. Could you please make it private[tree] though?
Clarification for 1.: By "array of bins," do you mean the array of classes to calculate the prediction (for classification)? Unless there are a very large number of classes, I do not think the savings will be that much.
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55135444
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20105/consoleFull) for PR 2332 at commit [`f1d11d1`](https://github.com/apache/spark/commit/f1d11d15fe519f9ef9d4e1158b309dc6af38864e).
* This patch **fails** 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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55056433
@jkbradley, yes, by "array of bins", I meaned that array of counts of each class, sorry for the missleading.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55177556
test this please
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55181133
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20116/consoleFull) for PR 2332 at commit [`f1d11d1`](https://github.com/apache/spark/commit/f1d11d15fe519f9ef9d4e1158b309dc6af38864e).
* This patch merges cleanly.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55188449
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/23/consoleFull) for PR 2332 at commit [`f1d11d1`](https://github.com/apache/spark/commit/f1d11d15fe519f9ef9d4e1158b309dc6af38864e).
* This patch **passes** 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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17342007
--- Diff: mllib/src/test/scala/org/apache/spark/mllib/tree/DecisionTreeSuite.scala ---
@@ -674,6 +676,45 @@ class DecisionTreeSuite extends FunSuite with LocalSparkContext {
validateClassifier(model, arr, 0.6)
}
+ test("split must satisfy min instances per node requirements") {
--- End diff --
Yes, that was what I had in mind.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55122746
@jkbradley thanks for your replies. as I replied in your comments, I have changed `minInstancePerNode` to 2 in test cases, and add one more test case to test that when a split doesn't satisfy min instances per node requirements, this split will not be chosen, even though the info gain is large(in this test case, total number of instances is 4, and we can find a split to let both left and right child have 2 instances). Do you think this is 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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55179528
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/23/consoleFull) for PR 2332 at commit [`f1d11d1`](https://github.com/apache/spark/commit/f1d11d15fe519f9ef9d4e1158b309dc6af38864e).
* This patch merges cleanly.
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/2332
---
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: [SPARK-2207][SPARK-3272][MLLib]Add minimum inf...
Posted by mengxr <gi...@git.apache.org>.
Github user mengxr commented on the pull request:
https://github.com/apache/spark/pull/2332#issuecomment-55194965
Merged into master. 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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by chouqin <gi...@git.apache.org>.
Github user chouqin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17334970
--- Diff: mllib/src/test/scala/org/apache/spark/mllib/tree/DecisionTreeSuite.scala ---
@@ -674,6 +676,45 @@ class DecisionTreeSuite extends FunSuite with LocalSparkContext {
validateClassifier(model, arr, 0.6)
}
+ test("split must satisfy min instances per node requirements") {
--- End diff --
Do you mean we should also check if we get correct result when `minInstancesPerNode` is 1 and 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: [SPARK-2207][SPARK-3272]Add minimum informatio...
Posted by jkbradley <gi...@git.apache.org>.
Github user jkbradley commented on a diff in the pull request:
https://github.com/apache/spark/pull/2332#discussion_r17321591
--- Diff: mllib/src/test/scala/org/apache/spark/mllib/tree/DecisionTreeSuite.scala ---
@@ -674,6 +676,45 @@ class DecisionTreeSuite extends FunSuite with LocalSparkContext {
validateClassifier(model, arr, 0.6)
}
+ test("split must satisfy min instances per node requirements") {
--- End diff --
Reasonable test, but could it check edge cases (such as when minInstancesPerNode is 1 vs. 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