-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
[SPARK-18592][ML] Move DT/RF/GBT Param setter methods to subclasses #16017
Changes from all commits
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 |
---|---|---|
|
@@ -52,33 +52,49 @@ class DecisionTreeClassifier @Since("1.4.0") ( | |
|
||
// Override parameter setters from parent trait for Java API compatibility. | ||
|
||
/** @group setParam */ | ||
@Since("1.4.0") | ||
override def setMaxDepth(value: Int): this.type = super.setMaxDepth(value) | ||
override def setMaxDepth(value: Int): this.type = set(maxDepth, value) | ||
|
||
/** @group setParam */ | ||
@Since("1.4.0") | ||
override def setMaxBins(value: Int): this.type = super.setMaxBins(value) | ||
override def setMaxBins(value: Int): this.type = set(maxBins, value) | ||
|
||
/** @group setParam */ | ||
@Since("1.4.0") | ||
override def setMinInstancesPerNode(value: Int): this.type = | ||
super.setMinInstancesPerNode(value) | ||
override def setMinInstancesPerNode(value: Int): this.type = set(minInstancesPerNode, value) | ||
|
||
/** @group setParam */ | ||
@Since("1.4.0") | ||
override def setMinInfoGain(value: Double): this.type = super.setMinInfoGain(value) | ||
override def setMinInfoGain(value: Double): this.type = set(minInfoGain, value) | ||
|
||
/** @group expertSetParam */ | ||
@Since("1.4.0") | ||
override def setMaxMemoryInMB(value: Int): this.type = super.setMaxMemoryInMB(value) | ||
override def setMaxMemoryInMB(value: Int): this.type = set(maxMemoryInMB, value) | ||
|
||
/** @group expertSetParam */ | ||
@Since("1.4.0") | ||
override def setCacheNodeIds(value: Boolean): this.type = super.setCacheNodeIds(value) | ||
override def setCacheNodeIds(value: Boolean): this.type = set(cacheNodeIds, value) | ||
|
||
/** | ||
* Specifies how often to checkpoint the cached node IDs. | ||
* 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. | ||
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. Hi all, this might be too trivial but I just want to let you know we probably should write other ones for |
||
* (default = 10) | ||
* @group setParam | ||
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. just curious, repeating this is kinda tedious - I thought it inherits the doc from the super, does it not? 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 point: we can inherit docs. 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 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 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. Right, that would make the setter appear in models. Your way is better. |
||
*/ | ||
@Since("1.4.0") | ||
override def setCheckpointInterval(value: Int): this.type = super.setCheckpointInterval(value) | ||
override def setCheckpointInterval(value: Int): this.type = set(checkpointInterval, value) | ||
|
||
/** @group setParam */ | ||
@Since("1.4.0") | ||
override def setImpurity(value: String): this.type = super.setImpurity(value) | ||
override def setImpurity(value: String): this.type = set(impurity, value) | ||
|
||
/** @group setParam */ | ||
@Since("1.6.0") | ||
override def setSeed(value: Long): this.type = super.setSeed(value) | ||
override def setSeed(value: Long): this.type = set(seed, value) | ||
|
||
override protected def train(dataset: Dataset[_]): DecisionTreeClassificationModel = { | ||
val categoricalFeatures: Map[Int, Int] = | ||
|
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 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).
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 checked the generated documentation and found it works well. Could you let me know what you found and how to reproduce it? Thanks.
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.
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.
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.
Btw, I created http://issues.apache.org/jira/browse/SPARK-18692 for monitoring Java 8 unidoc in the future
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 support that idea too! Thanks for your comment.