-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-18481][ML] ML 2.1 QA: Remove deprecated methods for ML #15913
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 6 commits
ecddf15
9a8c926
13988b4
7beb568
36c2e24
8d3c47a
0c85fd4
1d8cef5
3c8ee2d
c874d0e
157acf9
864be6e
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 |
|---|---|---|
|
|
@@ -183,6 +183,11 @@ class GBTRegressionModel private[ml]( | |
| @Since("1.4.0") | ||
| override def trees: Array[DecisionTreeRegressionModel] = _trees | ||
|
|
||
| /** | ||
| * Number of trees in ensemble | ||
| */ | ||
| val getNumTrees: Int = trees.length | ||
|
Contributor
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. Should this have an
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. Added. |
||
|
|
||
| @Since("1.4.0") | ||
| override def treeWeights: Array[Double] = _treeWeights | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -317,8 +317,28 @@ private[ml] trait TreeEnsembleParams extends DecisionTreeParams { | |
| } | ||
| } | ||
|
|
||
| /** Used for [[RandomForestParams]] */ | ||
| private[ml] trait HasFeatureSubsetStrategy extends Params { | ||
| /** | ||
| * Parameters for Random Forest algorithms. | ||
| */ | ||
| private[ml] trait RandomForestParams extends TreeEnsembleParams { | ||
|
|
||
| /** | ||
| * Number of trees to train (>= 1). | ||
| * If 1, then no bootstrapping is used. If > 1, then bootstrapping is done. | ||
| * TODO: Change to always do bootstrapping (simpler). SPARK-7130 | ||
| * (default = 20) | ||
| * @group param | ||
| */ | ||
| final val numTrees: IntParam = new IntParam(this, "numTrees", "Number of trees to train (>= 1)", | ||
|
Contributor
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. Note: the reason that we cannot add this to both GBT and RF (i.e. in
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. Good suggestion, added. |
||
| ParamValidators.gtEq(1)) | ||
|
|
||
| setDefault(numTrees -> 20) | ||
|
|
||
| /** @group setParam */ | ||
| def setNumTrees(value: Int): this.type = set(numTrees, value) | ||
|
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. Are these setter methods in traits Java-compatible?
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. Yeah, we already have |
||
|
|
||
| /** @group getParam */ | ||
| final def getNumTrees: Int = $(numTrees) | ||
|
|
||
| /** | ||
| * The number of features to consider for splits at each tree node. | ||
|
|
@@ -364,38 +384,6 @@ private[ml] trait HasFeatureSubsetStrategy extends Params { | |
| final def getFeatureSubsetStrategy: String = $(featureSubsetStrategy).toLowerCase | ||
| } | ||
|
|
||
| /** | ||
| * Used for [[RandomForestParams]]. | ||
| * This is separated out from [[RandomForestParams]] because of an issue with the | ||
| * `numTrees` method conflicting with this Param in the Estimator. | ||
| */ | ||
| private[ml] trait HasNumTrees extends Params { | ||
|
|
||
| /** | ||
| * Number of trees to train (>= 1). | ||
| * If 1, then no bootstrapping is used. If > 1, then bootstrapping is done. | ||
| * TODO: Change to always do bootstrapping (simpler). SPARK-7130 | ||
| * (default = 20) | ||
| * @group param | ||
| */ | ||
| final val numTrees: IntParam = new IntParam(this, "numTrees", "Number of trees to train (>= 1)", | ||
| ParamValidators.gtEq(1)) | ||
|
|
||
| setDefault(numTrees -> 20) | ||
|
|
||
| /** @group setParam */ | ||
| def setNumTrees(value: Int): this.type = set(numTrees, value) | ||
|
|
||
| /** @group getParam */ | ||
| final def getNumTrees: Int = $(numTrees) | ||
| } | ||
|
|
||
| /** | ||
| * Parameters for Random Forest algorithms. | ||
| */ | ||
| private[ml] trait RandomForestParams extends TreeEnsembleParams | ||
| with HasFeatureSubsetStrategy with HasNumTrees | ||
|
|
||
| private[spark] object RandomForestParams { | ||
| // These options should be lowercase. | ||
| final val supportedFeatureSubsetStrategies: Array[String] = | ||
|
|
@@ -405,15 +393,9 @@ private[spark] object RandomForestParams { | |
| private[ml] trait RandomForestClassifierParams | ||
| extends RandomForestParams with TreeClassifierParams | ||
|
|
||
| private[ml] trait RandomForestClassificationModelParams extends TreeEnsembleParams | ||
| with HasFeatureSubsetStrategy with TreeClassifierParams | ||
|
|
||
| private[ml] trait RandomForestRegressorParams | ||
| extends RandomForestParams with TreeRegressorParams | ||
|
|
||
| private[ml] trait RandomForestRegressionModelParams extends TreeEnsembleParams | ||
| with HasFeatureSubsetStrategy with TreeRegressorParams | ||
|
|
||
| /** | ||
| * Parameters for Gradient-Boosted Tree algorithms. | ||
| * | ||
|
|
@@ -443,12 +425,11 @@ private[ml] trait GBTParams extends TreeEnsembleParams with HasMaxIter with HasS | |
| * (default = 0.1) | ||
| * @group setParam | ||
| */ | ||
| def setStepSize(value: Double): this.type = set(stepSize, value) | ||
|
|
||
| override def validateParams(): Unit = { | ||
| def setStepSize(value: Double): this.type = { | ||
| require(ParamValidators.inRange(0, 1, lowerInclusive = false, upperInclusive = true)( | ||
|
Contributor
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'm not sure this is where we want to do this? The deprecation warning said to move checks from here into
Contributor
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's better to fail at set time rather than when
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. The original However,
Contributor
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. That makes more sense :) +1
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. This validation should really be in the Param itself. It's not since it's a sharedParam, but perhaps we should just copy the shared Param code here. The problem with putting validation in the setter is that Params can get set in other ways too (such as |
||
| getStepSize), "GBT parameter stepSize should be in interval (0, 1], " + | ||
| s"but it given invalid value $getStepSize.") | ||
| value), "GBT parameter stepSize should be in interval (0, 1], " + | ||
| s"but it given invalid value $value.") | ||
|
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 given invalid value" -> "it was given an invalid value" |
||
| set(stepSize, value) | ||
| } | ||
|
|
||
| /** (private[ml]) Create a BoostingStrategy instance to use with the old API. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,7 +78,14 @@ def overwrite(self): | |
| raise NotImplementedError("MLWriter is not yet implemented for type: %s" % type(self)) | ||
|
|
||
| def context(self, sqlContext): | ||
| """Sets the SQL context to use for saving.""" | ||
| """ | ||
| Sets the SQL context to use for saving. | ||
| .. note:: Deprecated in 2.1, use session instead. | ||
|
Contributor
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 we remove it from Scala in 2.2, then we have to remove it here in 2.2 right? I guess we should note it.
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. Added. |
||
| """ | ||
| raise NotImplementedError("MLWriter is not yet implemented for type: %s" % type(self)) | ||
|
|
||
| def session(self, sparkSession): | ||
| """Sets the Spark Session to use for saving.""" | ||
| raise NotImplementedError("MLWriter is not yet implemented for type: %s" % type(self)) | ||
|
|
||
|
|
||
|
|
@@ -105,10 +112,19 @@ def overwrite(self): | |
| return self | ||
|
|
||
| def context(self, sqlContext): | ||
| """Sets the SQL context to use for saving.""" | ||
| """ | ||
| Sets the SQL context to use for saving. | ||
| .. note:: Deprecated in 2.1, use session instead. | ||
| """ | ||
| warnings.warn("Deprecated in 2.1, use session instead.") | ||
| self._jwrite.context(sqlContext._ssql_ctx) | ||
| return self | ||
|
|
||
| def session(self, sparkSession): | ||
| """Sets the Spark Session to use for saving.""" | ||
| self._jwrite.session(sparkSession._jsparkSession) | ||
| return self | ||
|
|
||
|
|
||
| @inherit_doc | ||
| class MLWritable(object): | ||
|
|
@@ -155,7 +171,14 @@ def load(self, path): | |
| raise NotImplementedError("MLReader is not yet implemented for type: %s" % type(self)) | ||
|
|
||
| def context(self, sqlContext): | ||
| """Sets the SQL context to use for loading.""" | ||
| """ | ||
| Sets the SQL context to use for loading. | ||
| .. note:: Deprecated in 2.1, use session instead. | ||
| """ | ||
| raise NotImplementedError("MLReader is not yet implemented for type: %s" % type(self)) | ||
|
|
||
| def session(self, sparkSession): | ||
| """Sets the Spark Session to use for loading.""" | ||
| raise NotImplementedError("MLReader is not yet implemented for type: %s" % type(self)) | ||
|
|
||
|
|
||
|
|
@@ -180,10 +203,19 @@ def load(self, path): | |
| return self._clazz._from_java(java_obj) | ||
|
|
||
| def context(self, sqlContext): | ||
| """Sets the SQL context to use for loading.""" | ||
| """ | ||
| Sets the SQL context to use for loading. | ||
| .. note:: Deprecated in 2.1, use session instead. | ||
| """ | ||
| warnings.warn("Deprecated in 2.1, use session instead.") | ||
| self._jread.context(sqlContext._ssql_ctx) | ||
| return self | ||
|
|
||
| def session(self, sparkSession): | ||
| """Sets the Spark Session to use for loading.""" | ||
| self._jread.session(sparkSession._jsparkSession) | ||
| return self | ||
|
|
||
| @classmethod | ||
| def _java_loader_class(cls, clazz): | ||
| """ | ||
|
|
||
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.
Is this actually necessary? Before,
validateParams()was never used. Seems like we could just remove it.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 it's a bug that
validateParamswas never used. It should validate params interaction before fitting(if necessary), this is why we deprecatevalidateParamsand move what it does totransformSchema. We do not have corresponding test cases before, so no test was broken when we deprecatedvalidateParams. I added test cases in this PR.