-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-13677][ML] Implement Tree-Based Feature Transformation for ML #25383
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
Closed
Closed
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
95b1d22
init
zhengruifeng c17af00
xxx
zhengruifeng 92c2c86
init
zhengruifeng c0ba410
add testsuites
zhengruifeng 8b297ff
update transform
zhengruifeng 13027cb
update transform II
zhengruifeng e46d3a1
nit
zhengruifeng 8709ed1
move predictLeaf in to superclass
zhengruifeng eda4192
add some comments
zhengruifeng a4e60e3
add trait TreeEnsembleClassifierParams & TreeEnsembleRegressorParams
zhengruifeng dd68ac6
rename trait of impurity
zhengruifeng 1d59303
nit
zhengruifeng 3904602
revert trait renaming
zhengruifeng ae1cb9d
make test suites more common
zhengruifeng b21f8e8
nit
zhengruifeng d9d7368
update suites
zhengruifeng 5c5a76e
mv structure comment into the block
zhengruifeng 8247395
update
zhengruifeng 4eb97be
revert setVarianceCol to avoid mima
zhengruifeng File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,8 +34,8 @@ import org.apache.spark.ml.util.Instrumentation.instrumented | |
| import org.apache.spark.mllib.tree.configuration.{Algo => OldAlgo, Strategy => OldStrategy} | ||
| import org.apache.spark.mllib.tree.model.{DecisionTreeModel => OldDecisionTreeModel} | ||
| import org.apache.spark.rdd.RDD | ||
| import org.apache.spark.sql.{Dataset, Row} | ||
| import org.apache.spark.sql.functions.{col, lit} | ||
| import org.apache.spark.sql.{DataFrame, Dataset, Row} | ||
| import org.apache.spark.sql.functions.{col, lit, udf} | ||
| import org.apache.spark.sql.types.DoubleType | ||
|
|
||
| /** | ||
|
|
@@ -55,6 +55,10 @@ class DecisionTreeClassifier @Since("1.4.0") ( | |
|
|
||
| // Override parameter setters from parent trait for Java API compatibility. | ||
|
|
||
| /** @group setParam */ | ||
| @Since("3.0.0") | ||
| def setLeafCol(value: String): this.type = set(leafCol, value) | ||
|
|
||
| /** @group setParam */ | ||
| @Since("1.4.0") | ||
| def setMaxDepth(value: Int): this.type = set(maxDepth, value) | ||
|
|
@@ -136,8 +140,8 @@ class DecisionTreeClassifier @Since("1.4.0") ( | |
| val strategy = getOldStrategy(categoricalFeatures, numClasses) | ||
| instr.logNumClasses(numClasses) | ||
| instr.logParams(this, labelCol, featuresCol, predictionCol, rawPredictionCol, | ||
| probabilityCol, maxDepth, maxBins, minInstancesPerNode, minInfoGain, maxMemoryInMB, | ||
| cacheNodeIds, checkpointInterval, impurity, seed) | ||
| probabilityCol, leafCol, maxDepth, maxBins, minInstancesPerNode, minInfoGain, | ||
| maxMemoryInMB, cacheNodeIds, checkpointInterval, impurity, seed) | ||
|
|
||
| val trees = RandomForest.run(instances, strategy, numTrees = 1, featureSubsetStrategy = "all", | ||
| seed = $(seed), instr = Some(instr), parentUID = Some(uid)) | ||
|
|
@@ -210,6 +214,22 @@ class DecisionTreeClassificationModel private[ml] ( | |
| rootNode.predictImpl(features).prediction | ||
| } | ||
|
|
||
| /** @group setParam */ | ||
| @Since("3.0.0") | ||
| def setLeafCol(value: String): this.type = set(leafCol, value) | ||
|
|
||
| override def transform(dataset: Dataset[_]): DataFrame = { | ||
| transformSchema(dataset.schema, logging = true) | ||
|
|
||
| if ($(leafCol).nonEmpty) { | ||
| val leafUDF = udf { features: Vector => predictLeaf(features) } | ||
|
srowen marked this conversation as resolved.
|
||
| super.transform(dataset) | ||
| .withColumn($(leafCol), leafUDF(col($(featuresCol)))) | ||
| } else { | ||
| super.transform(dataset) | ||
|
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. It's trivial but I guess you could avoid calling this in two places ... call it once and either return it, or the result of it with a new column. |
||
| } | ||
| } | ||
|
|
||
| override protected def predictRaw(features: Vector): Vector = { | ||
| Vectors.dense(rootNode.predictImpl(features).impurityStats.stats.clone()) | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think we can do some refactoring here, in order to dedup this. Can we add it to a trait?
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.
This way seems like mllib's convention that not add setter into the
xxxParam-like trait, likesetVarianceColinDecisionTreeRegressionModelThere 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.
Yes, agree, it shouldn't be in the model classes. So
DecisionTreeClassifierParamsdoesn't help. Hm... per the discussion below, I agree it's extra weight to refactor the common elements into a superclass of two decision tree classifiers, but it might well be worth it. It looks like it would save a few hundred lines of duplicated code? that would mitigate the concern about the large change here. I'm lightly in favor of going that way. I wouldn't do it for 10 lines of code or something.