[SPARK-18592][ML] Move DT/RF/GBT Param setter methods to subclasses #16017

yanboliang · 2016-11-26T15:57:08Z

What changes were proposed in this pull request?

Mainly two changes:

Move DT/RF/GBT Param setter methods to subclasses.
Deprecate corresponding setter methods in the model classes.

See discussion here #15913 (comment).

How was this patch tested?

Existing tests.

yanboliang · 2016-11-26T15:59:45Z

cc @jkbradley

SparkQA · 2016-11-26T17:03:57Z

Test build #69187 has finished for PR 16017 at commit 39cbf42.

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

felixcheung · 2016-11-26T20:00:14Z

mllib/src/main/scala/org/apache/spark/ml/classification/DecisionTreeClassifier.scala

+   * [[org.apache.spark.SparkContext]].
+   * Must be >= 1.
+   * (default = 10)
+   * @group setParam


just curious, repeating this is kinda tedious - I thought it inherits the doc from the super, does it not?

Good point: we can inherit docs.

The cause of this change was suggested at #15913 (comment) , since Param setter methods in traits used to have the wrong type in Java. We would like to remove the setter method from the trait and it also does not make sense to have it in the Model classes. We could put the setter method in each subclass and then deprecate the method in the Model classes.

So we will remove the setter methods from all traits(in release 2.2), then we can not inherit docs from the traits. BTW, the current change is consistent with other ML algorithms which inherit traits. I'd like to know whether i understand the problem correctly. Thanks. @jkbradley

Right, that would make the setter appear in models. Your way is better.

jkbradley · 2016-11-28T15:46:54Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

+   * @deprecated This method is deprecated and will be removed in 2.2.0.
+   * @group setParam
+   */
+  @deprecated("This method is deprecated and will be removed in 2.2.0.", "2.1.0")


Does this appear as deprecated in the Estimator classes? If so, then can you update the comment to make it clear it is only deprecated for Models?

Actually, if this is a problem, we could mark the setters in Models as deprecated and leave the treeParams alone since the treeParams traits are private.

This will not appear in estimator classes, only in models. This can be confirmed by the following ways:
The IDE will prompt deprecated methods only for models:

We can also confirm it in Scala API docs:
DecisionTreeClassifier

DecisionTreeClassificationModel

Great, thank you for checking!

jkbradley · 2016-11-28T16:31:04Z

LGTM pending a check on whether the deprecations affect Estimators in addition to Models
Thanks for the follow-up!

jkbradley · 2016-11-28T16:31:11Z

mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala

@@ -134,27 +150,31 @@ private[ml] trait DecisionTreeParams extends PredictorParams
  /** @group setParam */
  def setSeed(value: Long): this.type = set(seed, value)


Are you intentionally keeping setSeed in Models?

Oops, I missed it and will add it. Thanks.

SparkQA · 2016-11-29T07:12:39Z

Test build #69306 has started for PR 16017 at commit 30f5096.

yanboliang · 2016-11-29T08:12:38Z

Jenkins, test this please.

SparkQA · 2016-11-29T09:22:58Z

Test build #69313 has finished for PR 16017 at commit 30f5096.

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

jkbradley · 2016-11-29T19:18:43Z

LGTM
Merging with master and branch-2.1
Thanks!

## What changes were proposed in this pull request? Mainly two changes: * Move DT/RF/GBT Param setter methods to subclasses. * Deprecate corresponding setter methods in the model classes. See discussion here #15913 (comment). ## How was this patch tested? Existing tests. Author: Yanbo Liang <[email protected]> Closes #16017 from yanboliang/spark-18592. (cherry picked from commit 95f7985) Signed-off-by: Joseph K. Bradley <[email protected]>

HyukjinKwon · 2016-12-02T04:36:19Z

mllib/src/main/scala/org/apache/spark/ml/classification/DecisionTreeClassifier.scala

+   * E.g. 10 means that the cache will get checkpointed every 10 iterations.
+   * This is only used if cacheNodeIds is true and if the checkpoint directory is set in
+   * [[org.apache.spark.SparkContext]].
+   * Must be >= 1.


Hi all, this might be too trivial but I just want to let you know we probably should write other ones for < or > such as {@literal <} or {@literal >}. Please refer #16013 (comment).

## What changes were proposed in this pull request? Mainly two changes: * Move DT/RF/GBT Param setter methods to subclasses. * Deprecate corresponding setter methods in the model classes. See discussion here apache#15913 (comment). ## How was this patch tested? Existing tests. Author: Yanbo Liang <[email protected]> Closes apache#16017 from yanboliang/spark-18592.

HyukjinKwon · 2016-12-08T02:33:35Z

mllib/src/main/scala/org/apache/spark/ml/classification/DecisionTreeClassifier.scala

@@ -52,33 +52,49 @@ class DecisionTreeClassifier @Since("1.4.0") (

  // Override parameter setters from parent trait for Java API compatibility.

+  /** @group setParam */


I am sorry but please allow me to leave another trivial comment about documentation. I haven't checked the built documentation but it seems we should use multiple-lines in this case (see #13855).

I checked the generated documentation and found it works well. Could you let me know what you found and how to reproduce it? Thanks.

Oh, I should have built and checked it first closely by myself. Thank you for correcting me. I just assumed that single line comment breaks other rendering.

Btw, I created http://issues.apache.org/jira/browse/SPARK-18692 for monitoring Java 8 unidoc in the future

I support that idea too! Thanks for your comment.

## What changes were proposed in this pull request? Mainly two changes: * Move DT/RF/GBT Param setter methods to subclasses. * Deprecate corresponding setter methods in the model classes. See discussion here apache#15913 (comment). ## How was this patch tested? Existing tests. Author: Yanbo Liang <[email protected]> Closes apache#16017 from yanboliang/spark-18592.

yanboliang added 2 commits November 26, 2016 07:36

Put DT/RF/GBT setter methods into each subclass.

7e52a08

Deprecate DT/RF/GBT setter methods in the Model classes

39cbf42

yanboliang mentioned this pull request Nov 26, 2016

[SPARK-18481][ML] ML 2.1 QA: Remove deprecated methods for ML #15913

Closed

felixcheung reviewed Nov 26, 2016

View reviewed changes

jkbradley reviewed Nov 28, 2016

View reviewed changes

Add missing deprecated method.

30f5096

asfgit closed this in 95f7985 Nov 29, 2016

yanboliang deleted the spark-18592 branch November 30, 2016 02:44

HyukjinKwon reviewed Dec 2, 2016

View reviewed changes

HyukjinKwon reviewed Dec 8, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-18592][ML] Move DT/RF/GBT Param setter methods to subclasses #16017

[SPARK-18592][ML] Move DT/RF/GBT Param setter methods to subclasses #16017

yanboliang commented Nov 26, 2016

yanboliang commented Nov 26, 2016

SparkQA commented Nov 26, 2016

felixcheung Nov 26, 2016

jkbradley Nov 27, 2016

yanboliang Nov 28, 2016 •

edited

Loading

jkbradley Nov 28, 2016

jkbradley Nov 28, 2016

jkbradley Nov 28, 2016

yanboliang Nov 29, 2016 •

edited

Loading

jkbradley Nov 29, 2016

jkbradley commented Nov 28, 2016

jkbradley Nov 28, 2016

yanboliang Nov 29, 2016

SparkQA commented Nov 29, 2016

yanboliang commented Nov 29, 2016

SparkQA commented Nov 29, 2016

jkbradley commented Nov 29, 2016

HyukjinKwon Dec 2, 2016 •

edited

Loading

HyukjinKwon Dec 8, 2016 •

edited

Loading

yanboliang Dec 8, 2016

HyukjinKwon Dec 8, 2016 •

edited

Loading

jkbradley Dec 8, 2016

HyukjinKwon Dec 8, 2016

		@@ -134,27 +150,31 @@ private[ml] trait DecisionTreeParams extends PredictorParams
		/** @group setParam */
		def setSeed(value: Long): this.type = set(seed, value)

		@@ -52,33 +52,49 @@ class DecisionTreeClassifier @Since("1.4.0") (

		// Override parameter setters from parent trait for Java API compatibility.

		/** @group setParam */

[SPARK-18592][ML] Move DT/RF/GBT Param setter methods to subclasses #16017

[SPARK-18592][ML] Move DT/RF/GBT Param setter methods to subclasses #16017

Conversation

yanboliang commented Nov 26, 2016

What changes were proposed in this pull request?

How was this patch tested?

yanboliang commented Nov 26, 2016

SparkQA commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanboliang Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanboliang Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkbradley commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 29, 2016

yanboliang commented Nov 29, 2016

SparkQA commented Nov 29, 2016

jkbradley commented Nov 29, 2016

HyukjinKwon Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

HyukjinKwon Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanboliang Nov 28, 2016 •

edited

Loading

yanboliang Nov 29, 2016 •

edited

Loading

HyukjinKwon Dec 2, 2016 •

edited

Loading

HyukjinKwon Dec 8, 2016 •

edited

Loading

HyukjinKwon Dec 8, 2016 •

edited

Loading