[SPARK-15444][PySpark][ML][HotFix] Default value mismatch of param linkPredictionCol for GeneralizedLinearRegression

[viirya]

What changes were proposed in this pull request?

Default value mismatch of param linkPredictionCol for GeneralizedLinearRegression between PySpark and Scala. That is because default value conflict between #13106 and #13129. This causes ml.tests failed.

How was this patch tested?

Existing tests.

[viirya]

cc @MLnick@mengxr This is a hotfix. Please take a look. Thanks.

[MLnick]

@viirya ah thanks I should have picked up the discrepancy. cc @holdenk@yanboliang

[MLnick]

Perhaps we should then make the default in __init__ and setParams be None rather than ""?

[viirya]

I was not sure about this. So just following other similar params in other classes.

[viirya]

Sounds good. Let me update it.

[MLnick]

I think the setDefault change you have is fine - I mean the default
constructor param
On Fri, 20 May 2016 at 11:37, Liang-Chi Hsieh notifications@github.com
wrote:

In python/pyspark/ml/regression.py
#13220 (comment):

@@ -1312,8 +1312,7 @@ def init(self, labelCol="label", featuresCol="features", predictionCol="pred
super(GeneralizedLinearRegression, self).init()
self._java_obj = self._new_java_obj(
"org.apache.spark.ml.regression.GeneralizedLinearRegression", self.uid)

•  self._setDefault(family="gaussian", maxIter=25, tol=1e-6, regParam=0.0, solver="irls",
  

•  linkPredictionCol="")
  

•  self._setDefault(family="gaussian", maxIter=25, tol=1e-6, regParam=0.0, solver="irls")
  

I was not sure about this. So just following other similar params in other
classes.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/13220/files/132753838a1424f09d85003a4436bb7ce0bd7774#r64014350

[viirya]

Yea. Updated. Thanks.

[SparkQA]

Test build #58973 has finished for PR 13220 at commit 1327538.

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

[MLnick]

Fortunately it doesn't actually impact since there is a nonEmpty check on the Scala side, but we should definitely make it consistent.

[viirya]

@MLnick unit tests in ml.tests will be failed due to this. This should be the only impact I think.

[SparkQA]

Test build #58975 has finished for PR 13220 at commit 74c8bc8.

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

[viirya]

@MLnick Please see if this is fine for you now. Thanks.

[yanboliang]

This fix looks good. Thanks! @viirya

[viirya]

Thank you @yanboliang

[viirya]

ping @MLnick

[MLnick]

Merged to master/branch-2.0. Thanks @viirya!

[viirya]

Thank you @MLnick

[holdenk]

Thanks for fixing this @viirya :)

[mengxr]

@viirya Thanks for fixing this quickly! @MLnick If master is broken, we should revert the commit first to unblock others (especially during QA period). We can re-submit the PR with fix.