[SPARK-15113][PySpark][ML] Add missing num features num classes #12889

holdenk · 2016-05-04T06:41:25Z

What changes were proposed in this pull request?

Add missing numFeatures and numClasses to the wrapped Java models in PySpark ML pipelines. Also tag DecisionTreeClassificationModel as Expiremental to match Scala doc.

How was this patch tested?

Extended doctests

…tion.py and mark experimental class to match scala class

SparkQA · 2016-05-04T06:53:56Z

Test build #57726 has finished for PR 12889 at commit c1961ae.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class JavaClassificationModel(JavaPredictionModel):
- class LogisticRegressionModel(JavaModel, JavaClassificationModel, JavaMLWritable, JavaMLReadable):
- class DecisionTreeClassificationModel(DecisionTreeModel, JavaClassificationModel, JavaMLWritable,
- class NaiveBayesModel(JavaModel, JavaClassificationModel, JavaMLWritable, JavaMLReadable):
- class MultilayerPerceptronClassificationModel(JavaModel, JavaClassificationModel, JavaMLWritable,

holdenk · 2016-05-05T07:31:30Z

cc @yanboliang

MLnick · 2016-05-05T12:05:39Z

python/pyspark/ml/classification.py


 @inherit_doc
-class DecisionTreeClassificationModel(DecisionTreeModel, JavaMLWritable, JavaMLReadable):
+class DecisionTreeClassificationModel(DecisionTreeModel, JavaClassificationModel, JavaMLWritable,


@holdenk are we not missing out GBTClassificationModel, RandomForestClassificationModel in classification? I think GBT should just be JavaPredictionModel

RandomForestClassificationModel and NaiveBayesModel should be extended from JavaClassificationModel, GBTClassificationModel should be JavaPredictionModel.

Just curious to know why we don't expose numClasses in GBTClassificationModel. Do we not support multiclass currently, or is there some other reason?

yanboliang · 2016-05-06T07:01:31Z

numFeatures in PredictionModel at Scala side, so all classes extends from PredictionModel(at Scala side) should be mixed-in it.
numClasses in ClassificationModel at Scala side, so all classes extends from ClassificationModel(at Scala side) should be mixed-in it.

…sses

SparkQA · 2016-05-06T20:04:17Z

Test build #58023 has finished for PR 12889 at commit e3b01f5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class RandomForestClassificationModel(TreeEnsembleModels, JavaClassificationModel, JavaMLWritable,
- class GBTClassificationModel(TreeEnsembleModels, JavaPredictionModel, JavaMLWritable,
- class MultilayerPerceptronClassificationModel(JavaModel, JavaPredictionModel, JavaMLWritable,

holdenk · 2016-05-09T00:30:26Z

Updated the classification models that do the mixing in based on the current inheritance in Scala side. I can follow up with more regression changes if no one takes over updating regression.

SparkQA · 2016-05-13T23:01:52Z

Test build #58594 has finished for PR 12889 at commit f5c69f1.

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

SparkQA · 2016-05-24T05:09:01Z

Test build #59181 has finished for PR 12889 at commit 6e35559.

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

…es in scala GeneralizedLinearRegressionModel

SparkQA · 2016-05-24T06:39:47Z

Test build #59184 has finished for PR 12889 at commit 020c096.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class RandomForestRegressionModel(TreeEnsembleModels, JavaPredictionModel, JavaMLWritable,

holdenk · 2016-05-26T18:57:25Z

ping?

holdenk · 2016-05-27T18:16:03Z

ping @yanboliang ?

SparkQA · 2016-06-03T22:44:37Z

Test build #59966 has finished for PR 12889 at commit 45570f5.

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

…sses

SparkQA · 2016-07-05T20:33:26Z

Test build #61783 has finished for PR 12889 at commit 0d7defa.

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

MechCoder · 2016-07-20T23:41:57Z

Just LinearRegressionModel seems missing to me. LGTM otherwise.

…sses

SparkQA · 2016-07-26T17:49:26Z

Test build #62895 has finished for PR 12889 at commit 8a30c7a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class LinearRegressionModel(JavaModel, JavaPredictionModel, JavaMLWritable, JavaMLReadable):

holdenk · 2016-08-03T21:23:44Z

Do we have interest in merging now that 2.0 is out? (cc @davies @yanboliang @MLnick )? Would be nice to do before we start adding more models to the Python ML API.

holdenk · 2016-08-08T19:44:35Z

ping @MLnick ?

MechCoder · 2016-08-08T20:26:47Z

python/pyspark/ml/classification.py

+    To be mixed in with class:`pyspark.ml.JavaModel`
+    """
+
+    @property


Should be 2.1?

Ah yes good point, this PR has been around - I'll update it here and in the other new method too.

…sses

SparkQA · 2016-08-08T21:25:59Z

Test build #63376 has finished for PR 12889 at commit 9283e3d.

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

MLnick · 2016-08-10T08:18:55Z

mllib/src/main/scala/org/apache/spark/ml/regression/GeneralizedLinearRegression.scala

  override def write: MLWriter =
    new GeneralizedLinearRegressionModel.GeneralizedLinearRegressionModelWriter(this)
+
+  override val numFeatures: Int = coefficients.size


I think we'll need a @Since("2.1.0") on this

numFeatures has always been here its just been the default implementation - but I guess the since wouldn't be too confusing.

I think adding a Since might actually be somewhat counter intuitive - how about a Javadoc note which says that it is now defined for this model starting in 2.1.0?

Ok fair point - I don't feel super strongly about it.

We still need to add this don't we? Otherwise it is the only public method in this class that doesn't have it?

The base class has @Since("1.6.0") on the method - so it has been public since 1.6 already.

Is that reflected in the documentation?

YUp, e.g. LogisticRegressionModel, RandomForestClassificationModel etc

MLnick · 2016-08-10T08:21:36Z

LGTM pending small comment on adding a since annotation.

SparkQA · 2016-08-18T21:46:01Z

Test build #64016 has finished for PR 12889 at commit 5045f7f.

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

MLnick · 2016-08-22T08:13:38Z

jenkins retest this please

SparkQA · 2016-08-22T09:11:51Z

Test build #64185 has finished for PR 12889 at commit 5045f7f.

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

MLnick · 2016-08-22T10:22:32Z

Merged to master. Thanks!

## What changes were proposed in this pull request? Add missing `numFeatures` and `numClasses` to the wrapped Java models in PySpark ML pipelines. Also tag `DecisionTreeClassificationModel` as Expiremental to match Scala doc. ## How was this patch tested? Extended doctests Author: Holden Karau <holden@us.ibm.com> Closes apache#12889 from holdenk/SPARK-15113-add-missing-numFeatures-numClasses.

holdenk added 3 commits May 3, 2016 23:38

Add JavaPredictionModel to expose numFeatures for wrapped Java models

0ef9826

Add numFeatures to DecisionTreeModel in regression

256c550

Add numFeatures / numClasses to the wrapped Java models in classifica…

c1961ae

…tion.py and mark experimental class to match scala class

MLnick reviewed May 5, 2016
View reviewed changes

holdenk added 2 commits May 6, 2016 12:22

Merge branch 'master' into SPARK-15113-add-missing-numFeatures-numCla…

beb6920

…sses

update more models to use the base methods

e3b01f5

Merge in master

f5c69f1

Merge in master

6e35559

holdenk added 2 commits May 23, 2016 22:57

Add numFeatures to the remainder of regression and override numFeatur…

a127e7c

…es in scala GeneralizedLinearRegressionModel

pep8

020c096

Merge in master

45570f5

Merge branch 'master' into SPARK-15113-add-missing-numFeatures-numCla…

0d7defa

…sses

MLnick mentioned this pull request Jul 18, 2016

[SPARK-11938][PySpark] Expose numFeatures in all ML PredictionModel for PySpark #13922

Closed

holdenk added 2 commits July 25, 2016 17:18

Merge branch 'master' into SPARK-15113-add-missing-numFeatures-numCla…

a15c2a4

…sses

Add numFeatures to LinearRegressionModel

8a30c7a

MechCoder reviewed Aug 8, 2016
View reviewed changes

holdenk added 2 commits August 8, 2016 13:29

Merge branch 'master' into SPARK-15113-add-missing-numFeatures-numCla…

c0f2e80

…sses

Did not make the 2.0 window - bump since for the new methods to 2.1

9283e3d

MLnick reviewed Aug 10, 2016
View reviewed changes

Merge in master

5045f7f

asfgit closed this in b264cbb Aug 22, 2016

[SPARK-15113][PySpark][ML] Add missing num features num classes #12889

[SPARK-15113][PySpark][ML] Add missing num features num classes #12889

Uh oh!

Conversation

holdenk commented May 4, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 4, 2016

Uh oh!

holdenk commented May 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanboliang commented May 6, 2016

Uh oh!

SparkQA commented May 6, 2016

Uh oh!

holdenk commented May 9, 2016

Uh oh!

SparkQA commented May 13, 2016

Uh oh!

SparkQA commented May 24, 2016

Uh oh!

SparkQA commented May 24, 2016

Uh oh!

holdenk commented May 26, 2016

Uh oh!

holdenk commented May 27, 2016

Uh oh!

SparkQA commented Jun 3, 2016

Uh oh!

SparkQA commented Jul 5, 2016

Uh oh!

MechCoder commented Jul 20, 2016

Uh oh!

SparkQA commented Jul 26, 2016

Uh oh!

holdenk commented Aug 3, 2016

Uh oh!

holdenk commented Aug 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MLnick commented Aug 10, 2016

Uh oh!

SparkQA commented Aug 18, 2016

Uh oh!

MLnick commented Aug 22, 2016

Uh oh!

SparkQA commented Aug 22, 2016

Uh oh!

MLnick commented Aug 22, 2016

Uh oh!

Reviewers

Assignees