-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-30144][ML][PySpark] Make MultilayerPerceptronClassificationModel extend MultilayerPerceptronParams #26838
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 2 commits
fc2cc5a
09bca1e
14ce378
7590bf8
f98de6b
2844d79
6be731d
7a98ffb
fdfeb6b
94b51a7
1833754
07267ff
fa1797e
40fc5da
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 |
|---|---|---|
|
|
@@ -273,29 +273,29 @@ object MultilayerPerceptronClassifier | |
| * Each layer has sigmoid activation function, output layer has softmax. | ||
| * | ||
| * @param uid uid | ||
| * @param layers array of layer sizes including input and output layers | ||
| * @param modelLayers array of layer sizes including input and output layers | ||
| * @param weights the weights of layers | ||
| */ | ||
| @Since("1.5.0") | ||
| class MultilayerPerceptronClassificationModel private[ml] ( | ||
| @Since("1.5.0") override val uid: String, | ||
| @Since("1.5.0") val layers: Array[Int], | ||
|
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. Shall we update migration guild?
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. @srowen Sean, this question is for you. |
||
| @Since("1.5.0") val modelLayers: Array[Int], | ||
|
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. same question, why need to change this?
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. oh, i see. MultilayerPerceptronParams has layers too?
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. Can we rename layers of MultilayerPerceptronParams 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. but renaming layers of MultilayerPerceptronParams will also break the API. |
||
| @Since("2.0.0") val weights: Vector) | ||
| extends ProbabilisticClassificationModel[Vector, MultilayerPerceptronClassificationModel] | ||
| with Serializable with MLWritable { | ||
| with MultilayerPerceptronParams with Serializable with MLWritable { | ||
|
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. Not related to this change. But do we use MultilayerPerceptronClassificationModel in executors? Like not every classification model extends Serializable.
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 am not sure about this. Seems only the tree related model extends Serializable. |
||
|
|
||
| @Since("1.6.0") | ||
| override val numFeatures: Int = layers.head | ||
| override val numFeatures: Int = modelLayers.head | ||
|
|
||
| private[ml] val mlpModel = FeedForwardTopology | ||
| .multiLayerPerceptron(layers, softmaxOnTop = true) | ||
| .multiLayerPerceptron(modelLayers, softmaxOnTop = true) | ||
| .model(weights) | ||
|
|
||
| /** | ||
| * Returns layers in a Java List. | ||
| */ | ||
| private[ml] def javaLayers: java.util.List[Int] = { | ||
| layers.toList.asJava | ||
| modelLayers.toList.asJava | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -308,7 +308,8 @@ class MultilayerPerceptronClassificationModel private[ml] ( | |
|
|
||
| @Since("1.5.0") | ||
| override def copy(extra: ParamMap): MultilayerPerceptronClassificationModel = { | ||
| val copied = new MultilayerPerceptronClassificationModel(uid, layers, weights).setParent(parent) | ||
| val copied = new MultilayerPerceptronClassificationModel(uid, modelLayers, weights) | ||
| .setParent(parent) | ||
| copyValues(copied, extra) | ||
| } | ||
|
|
||
|
|
@@ -322,11 +323,11 @@ class MultilayerPerceptronClassificationModel private[ml] ( | |
|
|
||
| override protected def predictRaw(features: Vector): Vector = mlpModel.predictRaw(features) | ||
|
|
||
| override def numClasses: Int = layers.last | ||
| override def numClasses: Int = modelLayers.last | ||
|
|
||
| @Since("3.0.0") | ||
| override def toString: String = { | ||
| s"MultilayerPerceptronClassificationModel: uid=$uid, numLayers=${layers.length}, " + | ||
| s"MultilayerPerceptronClassificationModel: uid=$uid, numLayers=${modelLayers.length}, " + | ||
| s"numClasses=$numClasses, numFeatures=$numFeatures" | ||
| } | ||
| } | ||
|
|
@@ -353,7 +354,7 @@ object MultilayerPerceptronClassificationModel | |
| // Save metadata and Params | ||
| DefaultParamsWriter.saveMetadata(instance, path, sc) | ||
| // Save model data: layers, weights | ||
|
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 layers now. |
||
| val data = Data(instance.layers, instance.weights) | ||
| val data = Data(instance.modelLayers, instance.weights) | ||
| val dataPath = new Path(path, "data").toString | ||
| sparkSession.createDataFrame(Seq(data)).repartition(1).write.parquet(dataPath) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -328,6 +328,9 @@ object MimaExcludes { | |
| // [SPARK-26457] Show hadoop configurations in HistoryServer environment tab | ||
| ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.status.api.v1.ApplicationEnvironmentInfo.this"), | ||
|
|
||
| // [SPARK-30144][ML] Make MultilayerPerceptronClassificationModel extend MultilayerPerceptronParams | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.ml.classification.MultilayerPerceptronClassificationModel.layers"), | ||
|
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. Just a question. Is this worth to break the API, @huaxingao ? |
||
|
|
||
| // Data Source V2 API changes | ||
| (problem: Problem) => problem match { | ||
| case MissingClassProblem(cls) => | ||
|
|
||
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 know the constructor is private, but is it necessary to change this name?
Uh oh!
There was an error while loading. Please reload this page.
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 is needed, since all estimators and their models should share the same params, and there is by chance a param named
layers...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.
What about just removing
modelLayersin model?since the value (array of layer sizes) can be easily obtain by $(layers)