[SPARK-10931][PYSPARK][ML] PySpark ML Models should contain Param values #14653

evanyc15 · 2016-08-15T21:59:19Z

What changes were proposed in this pull request?

Changed PySpark models to include the Param values.
Refer to the closed PR #10270 for additional information.

How was this patch tested?

Tested using Python doctests

Changesets:

Estimator UID is being copied correctly to the Transformer model objects and params now, working on Doctests

Changed the way parameters are copied from the Estimator to Transformer

Checkpoint, switching back to inheritance method

Working on DocTests

Implemented Doctests for Recommendation, Clustering, Classification (except RandomForestClassifier), Evaluation, Tuning, Regression (except RandomRegression)

Ready for Code Review

Code Review changeset #1

evanyc15 · 2016-08-15T22:02:31Z

CC @MLnick

MechCoder · 2016-08-17T23:16:20Z

python/pyspark/ml/classification.py

@@ -59,6 +59,16 @@ class LogisticRegression(JavaEstimator, HasFeaturesCol, HasLabelCol, HasPredicti
    ...     Row(label=0.0, weight=2.0, features=Vectors.sparse(1, [], []))]).toDF()
    >>> lr = LogisticRegression(maxIter=5, regParam=0.01, weightCol="weight")
    >>> model = lr.fit(df)
+    >>> emap = lr.extractParamMap()


style:
emap -> estimator_paramMap
mmap -> model_paramMap
?

MechCoder · 2016-08-17T23:57:49Z

Should we start having

PredictorParams -> (HasLabelCol, HasFeaturesCol, HasPredictionCol)
ClassifierParams -> (HasRawPredictionCol)

as done in the Scala side?

MechCoder · 2016-08-18T00:14:37Z

python/pyspark/ml/wrapper.py

@@ -243,7 +240,7 @@ def __init__(self, java_model=None):
        """
        Initialize this instance with a Java model object.
        Subclasses should call this constructor, initialize params,
-        and then call _transfer_params_from_java.
+        and then call _transformer_params.


Not sure you intended this change.

evanyc15 · 2016-08-18T21:13:18Z

@MechCoder I made the changes for emap -> estimator_paramMap, mmap -> model_paramMap, and (param, value) -> param, value.

BryanCutler · 2016-09-09T18:10:44Z

python/pyspark/ml/param/__init__.py

@@ -336,6 +336,11 @@ def hasParam(self, paramName):
            return isinstance(p, Param)
        else:
            raise TypeError("hasParam(): paramName must be a string")
+        try:


I don't think this code is reachable, is this necessary?

BryanCutler · 2016-09-09T18:17:41Z

Hey @evanyc15 , this is looking pretty good. I had a couple initial comments, but I'll have to look at it more in depth since there's a lot of changes. Mind resolving the conflicts first?

BryanCutler

@evanyc15 it looks good in general.

I think the doctests you have here would be better suited as unit tests because it's an area normal users would touch, and you could reduce some code duplication.

This PR might be easier for others to review if you pick a single estimator/model like LogisticRegression and demonstrate this change once before changing it everywhere.

BryanCutler · 2016-09-14T21:16:55Z

python/pyspark/ml/classification.py

+    >>> estimator_paramMap = lr.extractParamMap()
+    >>> model_paramMap = model.extractParamMap()
+    >>> all([estimator_paramMap[getattr(lr, param.name)] == value
+    ...     for param, value in model_paramMap.items()])


I think this comparison should be the other way around. Here you check that each param in the model is in the estimator, but it should be checking that each param in estimator made it to the model.

BryanCutler · 2016-09-14T21:19:08Z

python/pyspark/ml/classification.py

+    >>> all([param.parent == model.uid for param in model_paramMap])
+    True
+    >>> [param.name for param in model.params] # doctest: +NORMALIZE_WHITESPACE
+    ['elasticNetParam', 'featuresCol', 'fitIntercept', 'labelCol', 'maxIter',


I don't think we need this test, it's too brittle and doesn't really add much from the tests above

+1 for moving these kinds of tests to unit tests. Here, they make the documentation example confusing.

BryanCutler · 2016-09-14T21:25:10Z

python/pyspark/ml/classification.py

+class LogisticRegressionModel(JavaModel, HasFeaturesCol, HasLabelCol, HasPredictionCol, HasMaxIter,
+                              HasRegParam, HasTol, HasProbabilityCol, HasRawPredictionCol,
+                              HasElasticNetParam, HasFitIntercept, HasStandardization,
+                              HasThresholds, JavaMLWritable, JavaMLReadable):


I noticed this is missing a couple shared params like HasWeightCol

BryanCutler · 2016-09-14T21:45:49Z

python/pyspark/ml/classification.py

@@ -748,8 +785,9 @@ def _create_model(self, java_model):
        return RandomForestClassificationModel(java_model)


-class RandomForestClassificationModel(TreeEnsembleModel, JavaClassificationModel, JavaMLWritable,
-                                      JavaMLReadable):
+class RandomForestClassificationModel(TreeEnsembleModel, HasFeaturesCol, HasLabelCol,


I think this needs JavaClassificationModel

BryanCutler · 2016-09-14T21:51:29Z

python/pyspark/ml/classification.py

@@ -900,8 +947,8 @@ def getLossType(self):
        return self.getOrDefault(self.lossType)


-class GBTClassificationModel(TreeEnsembleModel, JavaPredictionModel, JavaMLWritable,
-                             JavaMLReadable):
+class GBTClassificationModel(TreeEnsembleModel, HasFeaturesCol, HasLabelCol, HasPredictionCol,


I think this needs JavaPredictionModel

BryanCutler · 2016-09-14T22:11:38Z

python/pyspark/ml/regression.py

@@ -560,6 +586,7 @@ class TreeRegressorParams(Params):
    """

    supportedImpurities = ["variance"]
+    # a placeholder to make it appear in the generated doc


is this necessary? maybe just a empty line will do

BryanCutler · 2016-09-14T22:12:38Z

python/pyspark/ml/regression.py

-    Abstraction for Decision Tree models.
+class DecisionTreeModel(JavaModel, JavaPredictionModel,
+                        HasFeaturesCol, HasLabelCol, HasPredictionCol):
+    """Abstraction for Decision Tree models.


newline after quotes

BryanCutler · 2016-09-14T22:13:30Z

python/pyspark/ml/regression.py

-                                  JavaMLReadable):
+class RandomForestRegressionModel(TreeEnsembleModel, JavaPredictionModel, HasFeaturesCol,
+                                  HasLabelCol, HasPredictionCol,
+                                  JavaMLWritable, JavaMLReadable):


nit: you could probably merge these 2 lines

BryanCutler · 2016-09-14T22:14:32Z

python/pyspark/ml/regression.py

@@ -1116,7 +1183,7 @@ class AFTSurvivalRegression(JavaEstimator, HasFeaturesCol, HasLabelCol, HasPredi
    |  1.0|    [1.0]|   1.0|       1.0|
    |  0.0|(1,[],[])|   0.0|       1.0|
    +-----+---------+------+----------+
-    ...
+    <BLANKLINE>


not sure why this is needed

BryanCutler · 2016-09-14T22:19:11Z

python/pyspark/ml/wrapper.py

@@ -200,7 +197,6 @@ def _create_model(self, java_model):
    def _fit_java(self, dataset):
        """
        Fits a Java model to the input dataset.
-


these blank lines should stay. Even though they are private functions, that's how it's usually done for Sphinx I believe

BryanCutler · 2016-09-14T22:50:16Z

@evanyc15 it looks good in general.

I think the doctests you have here would be better suited as unit tests because it's not an area normal users would care about and it could reduce code duplication

It might also be easier for others to review this PR if you picked a single estimator/model and demonstrated this change once to get feedback before applying it everywhere.

BryanCutler · 2016-09-14T22:51:59Z

It might also be good to discuss grouping the param mixins, similar to how it's done in Scala, so that both the estimator and model can inherit from a single common trait. This way you could be sure they will contain the same shared params.

jkbradley · 2016-10-04T01:36:05Z

ok to test

Sorry for the delay on this, but it'd be great to fix now!

evanyc15 · 2016-10-04T19:34:57Z

Sounds good Joseph. I'll resolve the conflicts.

evanyc15 · 2016-10-04T21:34:04Z

@jkbradley Hey Joseph,

I've resolved the merge conflicts. Can you please test?

holdenk · 2016-10-07T18:56:41Z

Huh I'm not sure why jenkins isn't picking this up - @jkbradley or @davidnavas can you tell jenkins this is ok to test again?

davidnavas · 2016-10-10T16:36:26Z

@holdenk happy to help if I can, is there something a mere mortal like myself can accomplish? [dunno how to poke jenkins hereabouts]

SparkQA · 2016-10-12T14:28:09Z

Test build #3328 has finished for PR 14653 at commit e706c7e.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- class LogisticRegressionModel(JavaModel, HasFeaturesCol, HasLabelCol, HasPredictionCol, HasMaxIter,
- class DecisionTreeClassificationModel(DecisionTreeModel, JavaClassificationModel, HasFeaturesCol,
- class RandomForestClassificationModel(TreeEnsembleModel, HasFeaturesCol, HasLabelCol,
- class GBTClassificationModel(TreeEnsembleModel, HasFeaturesCol, HasLabelCol, HasPredictionCol,
- class NaiveBayesModel(JavaModel, HasFeaturesCol, HasLabelCol, HasPredictionCol, HasProbabilityCol,
- class MultilayerPerceptronClassificationModel(JavaModel, HasFeaturesCol, HasLabelCol,
- class GaussianMixtureModel(JavaModel, HasFeaturesCol, HasPredictionCol, HasMaxIter, HasTol, HasSeed,
- class KMeansModel(JavaModel, JavaMLWritable, JavaMLReadable, HasFeaturesCol,
- class BisectingKMeansModel(JavaModel, HasFeaturesCol, HasPredictionCol, HasMaxIter,
- class LDAModel(JavaModel, HasFeaturesCol, HasMaxIter, HasSeed, HasCheckpointInterval):
- class CountVectorizerModel(JavaModel, HasInputCol, HasOutputCol,
- class IDFModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class MaxAbsScalerModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class MinMaxScalerModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class StandardScalerModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class StringIndexerModel(JavaModel, HasInputCol, HasOutputCol, HasHandleInvalid,
- class VectorIndexerModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class Word2VecModel(JavaModel, HasStepSize, HasMaxIter, HasSeed, HasInputCol,
- class PCAModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class RFormulaModel(JavaModel, HasFeaturesCol, HasLabelCol, JavaMLReadable, JavaMLWritable):
- class ChiSqSelectorModel(JavaModel, HasFeaturesCol, HasOutputCol, HasLabelCol,
- class ALS(JavaEstimator, HasCheckpointInterval, HasMaxIter, HasPredictionCol,
- class ALSModel(JavaModel, HasPredictionCol, JavaMLWritable, JavaMLReadable):
- class LinearRegressionModel(JavaModel, JavaPredictionModel, HasFeaturesCol, HasLabelCol,
- class IsotonicRegressionModel(JavaModel, JavaMLWritable, JavaMLReadable,
- class DecisionTreeModel(JavaModel, JavaPredictionModel,
- class RandomForestRegressionModel(TreeEnsembleModel, JavaPredictionModel, HasFeaturesCol,
- class GBTRegressionModel(TreeEnsembleModel, JavaPredictionModel,
- class AFTSurvivalRegressionModel(JavaModel, HasFeaturesCol, HasLabelCol,
- class GeneralizedLinearRegressionModel(JavaModel, JavaPredictionModel, HasLabelCol, HasFeaturesCol,

holdenk · 2016-10-12T22:47:20Z

Looks like jenkins has picked it up. Maybe @evanyc15 can merge in master (or rebase on master) so jenkins re-runs and verify the tests?

evanyc15 · 2016-10-12T22:59:44Z

@holdenk I just rebased and pushed again. Hopefully, Jenkins passes this time

evanyc15 · 2016-10-14T00:00:49Z

@davidnavas
Hi David,
I have rebased and pushed again. Could you tell Jenkins to re-test the PR?
Thank you

davidnavas · 2016-10-14T00:24:02Z

Sadly, I don't have that superpower :( Leastwise not that I know. Perhaps Holden's appeal to @jkbradley was what worked last time?

MLnick · 2016-10-14T06:45:23Z

jenkins test this please

SparkQA · 2016-10-14T07:12:26Z

Test build #66953 has finished for PR 14653 at commit eb7ca31.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class LogisticRegressionModel(JavaModel, HasFeaturesCol, HasLabelCol, HasPredictionCol, HasMaxIter,
- class DecisionTreeClassificationModel(DecisionTreeModel, JavaClassificationModel, HasFeaturesCol,
- class RandomForestClassificationModel(TreeEnsembleModel, HasFeaturesCol, HasLabelCol,
- class GBTClassificationModel(TreeEnsembleModel, HasFeaturesCol, HasLabelCol, HasPredictionCol,
- class NaiveBayesModel(JavaModel, HasFeaturesCol, HasLabelCol, HasPredictionCol, HasProbabilityCol,
- class MultilayerPerceptronClassificationModel(JavaModel, HasFeaturesCol, HasLabelCol,
- class GaussianMixtureModel(JavaModel, HasFeaturesCol, HasPredictionCol, HasMaxIter, HasTol, HasSeed,
- class KMeansModel(JavaModel, JavaMLWritable, JavaMLReadable, HasFeaturesCol,
- class BisectingKMeansModel(JavaModel, HasFeaturesCol, HasPredictionCol, HasMaxIter,
- class LDAModel(JavaModel, HasFeaturesCol, HasMaxIter, HasSeed, HasCheckpointInterval):
- class CountVectorizerModel(JavaModel, HasInputCol, HasOutputCol,
- class IDFModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class MaxAbsScalerModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class MinMaxScalerModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class StandardScalerModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class StringIndexerModel(JavaModel, HasInputCol, HasOutputCol, HasHandleInvalid,
- class VectorIndexerModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class Word2VecModel(JavaModel, HasStepSize, HasMaxIter, HasSeed, HasInputCol,
- class PCAModel(JavaModel, HasInputCol, HasOutputCol, JavaMLReadable, JavaMLWritable):
- class RFormulaModel(JavaModel, HasFeaturesCol, HasLabelCol, JavaMLReadable, JavaMLWritable):
- class ChiSqSelectorModel(JavaModel, HasFeaturesCol, HasOutputCol, HasLabelCol,
- class ALS(JavaEstimator, HasCheckpointInterval, HasMaxIter, HasPredictionCol,
- class ALSModel(JavaModel, HasPredictionCol, JavaMLWritable, JavaMLReadable):
- class LinearRegressionModel(JavaModel, JavaPredictionModel, HasFeaturesCol, HasLabelCol,
- class IsotonicRegressionModel(JavaModel, JavaMLWritable, JavaMLReadable,
- class DecisionTreeModel(JavaModel, JavaPredictionModel,
- class RandomForestRegressionModel(TreeEnsembleModel, JavaPredictionModel, HasFeaturesCol,
- class GBTRegressionModel(TreeEnsembleModel, JavaPredictionModel,
- class AFTSurvivalRegressionModel(JavaModel, HasFeaturesCol, HasLabelCol,
- class GeneralizedLinearRegressionModel(JavaModel, JavaPredictionModel, HasLabelCol, HasFeaturesCol,

evanyc15 · 2016-10-18T17:42:55Z

@MLnick @jkbradley Do you mind merging the PR? Thank you

jkbradley · 2016-10-25T21:15:35Z

@evanyc15 OK back for real now...sorry for the delay. @BryanCutler has a lot of good comments. Could you please address them? Regarding splitting this up into multiple PRs, I strongly +1 that in general, though I'm OK if you want to do this as a batch.

I'll test this PR out now...

jkbradley · 2016-10-25T22:21:32Z

Having some trouble b/c the doc build was apparently broken 19 days ago. Looking into a fix now.

~ Doc build is being fixed...

jkbradley · 2016-10-26T00:39:14Z

One good set of unit tests might emulate ParamsSuite.checkParams from Scala. That tests several things which should be uniform across all Params subclasses.

evanyc15 · 2016-10-31T20:57:58Z

Hey @jkbradley the checkParams method already exists in the Python side. It's defined in the tests.py DefaultValuesTests class and is being called by test_java_params. I'm removing the param testing from the Python Doctests now and will be implementing the Unit test in one of the classes for now. Once approved, I will then implement the Unit test in the remaining classes.

Copied parameters over from Estimator to Transformer Estimator UID is being copied correctly to the Transformer model objects and params now, working on Doctests Changed the way parameters are copied from the Estimator to Transformer Checkpoint, switching back to inheritance method Working on DocTests Implemented Doctests for Recommendation, Clustering, Classification (except RandomForestClassifier), Evaluation, Tuning, Regression (except RandomRegression) Ready for Code Review Code Review changeset apache#1

AmplabJenkins · 2018-06-09T00:25:02Z

Can one of the admins verify this patch?

BryanCutler · 2018-06-13T16:39:56Z

@evanyc15 would you mind closing this PR? Thanks!

evanyc15 mentioned this pull request Aug 15, 2016

[SPARK-10931][PYSPARK][ML] PySpark ML Models should contain Param values #10270

Closed

MechCoder reviewed Aug 17, 2016
View reviewed changes

MechCoder reviewed Aug 18, 2016
View reviewed changes

evanyc15 force-pushed the SPARK-10931-pyspark-mllib branch from 2f9417c to 8d092e8 Compare August 18, 2016 21:07

BryanCutler reviewed Sep 9, 2016
View reviewed changes

evanyc15 force-pushed the SPARK-10931-pyspark-mllib branch 2 times, most recently from 8d7aedb to e12cbd7 Compare September 13, 2016 22:20

BryanCutler reviewed Sep 14, 2016

View reviewed changes

evanyc15 force-pushed the SPARK-10931-pyspark-mllib branch 2 times, most recently from fc11247 to e706c7e Compare October 4, 2016 21:33

evanyc15 force-pushed the SPARK-10931-pyspark-mllib branch from e706c7e to eb7ca31 Compare October 12, 2016 22:58

evanyc15 force-pushed the SPARK-10931-pyspark-mllib branch from eb7ca31 to 80303bf Compare October 25, 2016 23:06

evanyc15 force-pushed the SPARK-10931-pyspark-mllib branch from 80303bf to 05c11f4 Compare October 31, 2016 21:44

BryanCutler mentioned this pull request Feb 1, 2017

[SPARK-15009][PYTHON][ML] Construct a CountVectorizerModel from a vocabulary list #16770

Closed

HyukjinKwon mentioned this pull request Jul 16, 2018

[INFRA] Close stale PR #21781

Closed

asfgit closed this in 1a4fda8 Jul 19, 2018

[SPARK-10931][PYSPARK][ML] PySpark ML Models should contain Param values #14653

[SPARK-10931][PYSPARK][ML] PySpark ML Models should contain Param values #14653

Conversation

evanyc15 commented Aug 15, 2016 • edited

What changes were proposed in this pull request?

How was this patch tested?

Changesets:

evanyc15 commented Aug 15, 2016

Choose a reason for hiding this comment

MechCoder commented Aug 17, 2016

Choose a reason for hiding this comment

evanyc15 commented Aug 18, 2016

Choose a reason for hiding this comment

BryanCutler commented Sep 9, 2016

BryanCutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler Sep 14, 2016 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler commented Sep 14, 2016

BryanCutler commented Sep 14, 2016

jkbradley commented Oct 4, 2016

evanyc15 commented Oct 4, 2016

evanyc15 commented Oct 4, 2016 • edited

holdenk commented Oct 7, 2016

davidnavas commented Oct 10, 2016

SparkQA commented Oct 12, 2016

holdenk commented Oct 12, 2016

evanyc15 commented Oct 12, 2016

evanyc15 commented Oct 14, 2016

davidnavas commented Oct 14, 2016

MLnick commented Oct 14, 2016

SparkQA commented Oct 14, 2016

evanyc15 commented Oct 18, 2016

jkbradley commented Oct 25, 2016

jkbradley commented Oct 25, 2016 • edited

jkbradley commented Oct 26, 2016

evanyc15 commented Oct 31, 2016

AmplabJenkins commented Jun 9, 2018

BryanCutler commented Jun 13, 2018

evanyc15 commented Aug 15, 2016 •

edited

BryanCutler Sep 14, 2016 •

edited

evanyc15 commented Oct 4, 2016 •

edited

jkbradley commented Oct 25, 2016 •

edited