-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-9073][ML] spark.ml Models copy() should call setParent when there is a parent #7447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
25b4f5b
6a5b495
f091a6b
5316908
e0e3f22
b9aa325
04ccb0f
1e0b610
4c60f8d
5d798e9
392a7b2
9445cb7
73f78ac
200dd11
a6908fe
c01e93c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,6 +180,7 @@ private class MyLogisticRegressionModel( | |
| */ | ||
| override def copy(extra: ParamMap): MyLogisticRegressionModel = { | ||
| copyValues(new MyLogisticRegressionModel(uid, weights), extra) | ||
| .setParent(parent) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this can fit on 1 line, please do so. (here and elsewhere) |
||
| } | ||
| } | ||
| // scalastyle:on println | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ package org.apache.spark.ml.classification | |
|
|
||
| import org.apache.spark.SparkFunSuite | ||
| import org.apache.spark.ml.impl.TreeTests | ||
| import org.apache.spark.ml.param.ParamsSuite | ||
| import org.apache.spark.ml.param.{ParamMap, ParamsSuite} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no longer any need to import ParamMap. Please check imports in other tests too |
||
| import org.apache.spark.ml.tree.LeafNode | ||
| import org.apache.spark.mllib.linalg.{Vector, Vectors} | ||
| import org.apache.spark.mllib.regression.LabeledPoint | ||
|
|
@@ -232,6 +232,13 @@ class DecisionTreeClassifierSuite extends SparkFunSuite with MLlibTestSparkConte | |
| compareAPIs(rdd, dt, categoricalFeatures = Map.empty[Int, Int], numClasses) | ||
| } | ||
|
|
||
| test("copied model must have the same parent") { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need to write a new unit test for each model. A more efficient way would be to write a generic testing utility like this (maybe inside a new file like test/scala/org/apache/spark/ml/util/MLTestingUtils.scala): Later on, we can add other standard checks here as well.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then each unit test can call that helper in an existing test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better to make helper method in order not to make duplicates. But it is better to have one unit test has one assert to make it easy to specify which test fail. How about?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, that's a good point. I'm OK with you leaving them as separate tests. |
||
| ParamsSuite.checkParams(new DecisionTreeClassifier) | ||
| val model = new DecisionTreeClassificationModel("dtc", new LeafNode(0.0, 0.0, null), 2) | ||
| val copied = model.copy(ParamMap.empty) | ||
| assert(model.parent == copied.parent) | ||
| } | ||
|
|
||
| test("predictRaw and predictProbability") { | ||
| val rdd = continuousDataPointsForMulticlassRDD | ||
| val dt = new DecisionTreeClassifier() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix indentation