[SPARK-3381] [MLlib] Eliminate bins for unordered features in DecisionTrees #4231

MechCoder · 2015-01-27T20:47:52Z

For unordered features, it is sufficient to use splits since the threshold of the split corresponds the threshold of the HighSplit of the bin and there is no use of the LowSplit.

MechCoder · 2015-01-27T20:49:14Z

ping @jkbradley . Would be great if you could have a look.

SparkQA · 2015-01-27T20:52:47Z

Test build #26183 has started for PR 4231 at commit 6dfaa28.

This patch merges cleanly.

SparkQA · 2015-01-27T22:01:21Z

Test build #26183 has finished for PR 4231 at commit 6dfaa28.

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

AmplabJenkins · 2015-01-27T22:01:24Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26183/
Test PASSed.

MechCoder · 2015-01-28T04:32:54Z

mllib/src/main/scala/org/apache/spark/mllib/tree/impl/TreePoint.scala

@@ -96,14 +96,12 @@ private[tree] object TreePoint {
   * Find bin for one (labeledPoint, feature).
   *
   * @param featureArity  0 for continuous features; number of categories for categorical features.
-   * @param isUnorderedFeature  (only applies if feature is categorical)


@jkbradley I removed this param as it is unused. I don't think it is a problem since all tests pass.

Sounds good. Can you please also remove it from labeledPointToTreePoint and not compute it in convertToTreeRDD?

MechCoder · 2015-01-28T04:34:37Z

It also returns empty bins, just to be compatible with the present API.

MechCoder · 2015-01-29T19:41:31Z

@jkbradley Can I please get a pass or comments on this? Or maybe others who are familiar with the tree code (@mengxr )

MechCoder · 2015-01-30T19:08:12Z

Just out of curiosity, what happens when the deadline for code freeze passes?, i.e tomorrow.

jkbradley · 2015-01-30T20:33:16Z

@MechCoder Sorry, there are a lot of PRs out there now, so this may not get merged before the code freeze. It's a good cleanup, though, so I'll definitely take a look when I can. Thanks for your patience.

jkbradley · 2015-01-30T20:34:32Z

Oh, and the code freeze just means that we'll start work on cutting a release for 1.3 and no more PRs (unless really necessary) will get merged into the cut. PRs can still get merged into master (which will end up in 1.4).

MechCoder · 2015-01-31T19:21:36Z

Thanks :) Looking forward to your reviews. I will work on other stuff till then.

MechCoder · 2015-02-03T17:35:19Z

@jkbradley Sorry for being impatient, but would you be able to have a look anytime soon?

MechCoder · 2015-02-06T19:46:30Z

ping @jkbradley ?

jkbradley · 2015-02-06T19:59:24Z

I'll do my best to look at it today---I hope!

jkbradley · 2015-02-07T03:36:55Z

mllib/src/main/scala/org/apache/spark/mllib/tree/DecisionTree.scala

@@ -339,6 +340,7 @@ object DecisionTree extends Serializable with Logging {
      agg: DTStatsAggregator,
      treePoint: TreePoint,
      bins: Array[Array[Bin]],


You shouldn't need to pass bins to this method anymore.

jkbradley · 2015-02-07T03:37:36Z

Thanks for the PR. I’ve added a number of comments. Unfortunately, I won’t have an Internet connection for the next week, so I may not be able to respond to updates. Feel free to ping other people though, and I’ll check when I’m back!

MechCoder · 2015-02-07T04:09:53Z

@jkbradley Thanks for your reviews. I fixed them up.

Anyone else want to have a final look?
cc @manishamde @mengxr ?

SparkQA · 2015-02-07T04:12:58Z

Test build #26985 has started for PR 4231 at commit c7743c6.

This patch merges cleanly.

SparkQA · 2015-02-07T04:22:55Z

Test build #26986 has started for PR 4231 at commit aa30be9.

This patch merges cleanly.

SparkQA · 2015-02-07T05:27:13Z

Test build #26985 has finished for PR 4231 at commit c7743c6.

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

AmplabJenkins · 2015-02-07T05:27:17Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26985/
Test PASSed.

SparkQA · 2015-02-07T05:36:36Z

Test build #26986 has finished for PR 4231 at commit aa30be9.

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

AmplabJenkins · 2015-02-07T05:36:40Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26986/
Test PASSed.

jkbradley · 2015-02-17T00:36:13Z

mllib/src/main/scala/org/apache/spark/mllib/tree/DecisionTree.scala

+            // For unordered categorical features, there is no need to construct the bins.
+            // since there is a one-to-one correspondence between the splits and the bins.
+            bins(featureIndex) = new Array[Bin](0)
+            }


fix indentation

Do you mean to move the closing brace 2 spaces ahead?

The closing brace should be 2 spaces to the left: [https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide]

Oh, I see you got it.

MechCoder · 2015-02-17T02:45:21Z

@jkbradley fixed!

SparkQA · 2015-02-17T02:47:35Z

Test build #27605 has started for PR 4231 at commit b7c5581.

This patch merges cleanly.

SparkQA · 2015-02-17T03:40:14Z

Test build #27605 has finished for PR 4231 at commit b7c5581.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-17T03:40:18Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27605/
Test FAILed.

SparkQA · 2015-02-17T03:52:37Z

Test build #27608 has started for PR 4231 at commit 58c19a5.

This patch merges cleanly.

SparkQA · 2015-02-17T05:09:43Z

Test build #27608 has finished for PR 4231 at commit 58c19a5.

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

AmplabJenkins · 2015-02-17T05:09:47Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27608/
Test PASSed.

jkbradley · 2015-02-17T18:24:51Z

LGTM, thanks!

manishamde · 2015-02-17T18:29:03Z

@MechCoder Sorry I didn't see the message earlier. I am sure @jkbradley must have done a thorough review but please let me know if you need me to take a look.

MechCoder · 2015-02-17T18:32:48Z

@manishamde Thanks. The LGTM suggests that this should be good to go in! ;)

jkbradley · 2015-02-17T19:21:21Z

Merged into master--thanks!

manishamde · 2015-02-17T19:22:13Z

Thanks @MechCoder @jkbradley

MechCoder · 2015-02-18T06:31:52Z

Thanks ! Looking forward to learn lot's more

MechCoder reviewed Jan 28, 2015
View reviewed changes

MechCoder changed the title ~~[SPARK-3381] [MLlib] Eliminate bins for unordered features~~ [SPARK-3381] [MLlib] Eliminate bins for unordered features in DecisionTrees Jan 28, 2015

jkbradley reviewed Feb 7, 2015
View reviewed changes

jkbradley reviewed Feb 17, 2015
View reviewed changes

MechCoder added 4 commits February 17, 2015 09:21

[SPARK-3381] [MLlib] Eliminate bins for unordered features

d3ee042

COSMIT

b2b9b89

Remove unordered feature calculation in labeledPointToTreePoint

c274b74

COSMIT

58c19a5

MechCoder force-pushed the spark-3381 branch from b7c5581 to 58c19a5 Compare February 17, 2015 03:51

asfgit closed this in 9b746f3 Feb 17, 2015

MechCoder deleted the spark-3381 branch February 17, 2015 19:27

[SPARK-3381] [MLlib] Eliminate bins for unordered features in DecisionTrees #4231

[SPARK-3381] [MLlib] Eliminate bins for unordered features in DecisionTrees #4231

Conversation

MechCoder commented Jan 27, 2015

MechCoder commented Jan 27, 2015

SparkQA commented Jan 27, 2015

SparkQA commented Jan 27, 2015

AmplabJenkins commented Jan 27, 2015

MechCoder Jan 28, 2015

Choose a reason for hiding this comment

jkbradley Feb 7, 2015

Choose a reason for hiding this comment

MechCoder commented Jan 28, 2015

MechCoder commented Jan 29, 2015

MechCoder commented Jan 30, 2015

jkbradley commented Jan 30, 2015

jkbradley commented Jan 30, 2015

MechCoder commented Jan 31, 2015

MechCoder commented Feb 3, 2015

MechCoder commented Feb 6, 2015

jkbradley commented Feb 6, 2015

jkbradley Feb 7, 2015

Choose a reason for hiding this comment

jkbradley commented Feb 7, 2015

MechCoder commented Feb 7, 2015

SparkQA commented Feb 7, 2015

SparkQA commented Feb 7, 2015

SparkQA commented Feb 7, 2015

AmplabJenkins commented Feb 7, 2015

SparkQA commented Feb 7, 2015

AmplabJenkins commented Feb 7, 2015

jkbradley Feb 17, 2015

Choose a reason for hiding this comment

MechCoder Feb 17, 2015

Choose a reason for hiding this comment

jkbradley Feb 17, 2015

Choose a reason for hiding this comment

jkbradley Feb 17, 2015

Choose a reason for hiding this comment

MechCoder commented Feb 17, 2015

SparkQA commented Feb 17, 2015

SparkQA commented Feb 17, 2015

AmplabJenkins commented Feb 17, 2015

SparkQA commented Feb 17, 2015

SparkQA commented Feb 17, 2015

AmplabJenkins commented Feb 17, 2015

jkbradley commented Feb 17, 2015

manishamde commented Feb 17, 2015

MechCoder commented Feb 17, 2015

jkbradley commented Feb 17, 2015

manishamde commented Feb 17, 2015

MechCoder commented Feb 18, 2015