[SPARK-32310][ML][PySpark][3.0] ML params default value parity #29159

huaxingao · 2020-07-20T05:29:02Z

What changes were proposed in this pull request?

backporting the changes to 3.0
set params default values in trait Params for feature and tuning in both Scala and Python.

Why are the changes needed?

Make ML has the same default param values between estimator and its corresponding transformer, and also between Scala and Python.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing and modified tests

huaxingao · 2020-07-20T05:58:22Z

cc @srowen @viirya @zhengruifeng

SparkQA · 2020-07-20T06:13:41Z

Test build #126152 has finished for PR 29159 at commit f657d77.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-20T07:05:02Z

Test build #126158 has finished for PR 29159 at commit 16abf9c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

zhengruifeng · 2020-07-20T09:58:39Z

retest this please

SparkQA · 2020-07-20T10:57:10Z

Test build #126176 has finished for PR 29159 at commit 16abf9c.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2020-07-20T15:45:53Z

Jenkins retest this please

SparkQA · 2020-07-20T16:47:46Z

Test build #126189 has finished for PR 29159 at commit 16abf9c.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-20T16:58:19Z

retest this please

SparkQA · 2020-07-20T17:54:05Z

Test build #126192 has finished for PR 29159 at commit 16abf9c.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-20T18:11:13Z

retest this please

SparkQA · 2020-07-20T19:01:12Z

Test build #126193 has finished for PR 29159 at commit 16abf9c.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-20T20:27:40Z

retest this please

SparkQA · 2020-07-20T21:16:50Z

Test build #126201 has finished for PR 29159 at commit 16abf9c.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2020-07-20T21:25:57Z

retest this please

viirya · 2020-07-20T21:26:48Z

For backport PR, can you add [3.0] to the PR title?

viirya · 2020-07-20T21:30:10Z

Is this just a backport of #29112? Looks it it is bigger change than #29112?

huaxingao · 2020-07-20T21:52:06Z

@viirya It's backporting #29112 and #29153. I haven't finished #29153 yet. I want to finish 3.0 first because seems the plan is to cut 3.0.1 pretty soon.

SparkQA · 2020-07-20T22:50:36Z

Test build #126203 has finished for PR 29159 at commit 16abf9c.

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

Fokko · 2020-07-21T10:11:18Z

#29122 Isn't included in the PR AFAIK. I think it is a typo, it is #29112.

huaxingao · 2020-07-21T14:43:21Z

Sorry, fixed. @Fokko

zhengruifeng · 2020-07-22T05:26:34Z

mllib/src/test/scala/org/apache/spark/ml/util/DefaultReadWriteTest.scala

@@ -64,7 +64,12 @@ trait DefaultReadWriteTest extends TempDirectory { self: Suite =>
            case (Array(values), Array(newValues)) =>
              assert(values === newValues, s"Values do not match on param ${p.name}.")
            case (value, newValue) =>
-              assert(value === newValue, s"Values do not match on param ${p.name}.")
+              if (value.isInstanceOf[Double] && value.asInstanceOf[Double].isNaN) {


Why we need this check?

If necessary, may change to:

case (value: Double, newValue: Double) => assert(value.isNaN && newValue.isNaN || value == newValue, ...)

Before my change, this setDefault(strategy -> Imputer.mean, missingValue -> Double.NaN) is in Imputer. Now I moved this to ImputerParams so ImputerModel also has the default value of missingValue set to Double.NaN. In DefaultReadWriteTest, it compares the default value of missingValue in the original model and the loaded model, so I need to make the NaN comparison work.

SparkQA · 2020-07-22T07:05:02Z

Test build #126304 has finished for PR 29159 at commit 52dd9b4.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-22T14:53:25Z

retest this please

SparkQA · 2020-07-22T15:54:01Z

Test build #126342 has finished for PR 29159 at commit 52dd9b4.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-22T16:00:08Z

retest this please

SparkQA · 2020-07-22T17:13:29Z

Test build #126346 has finished for PR 29159 at commit 52dd9b4.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-22T17:20:17Z

retest this please

SparkQA · 2020-07-22T18:25:11Z

Test build #126351 has finished for PR 29159 at commit 52dd9b4.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-22T18:31:52Z

retest this please

SparkQA · 2020-07-22T19:34:17Z

Test build #126353 has finished for PR 29159 at commit 52dd9b4.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-22T19:49:04Z

retest this please

SparkQA · 2020-07-22T20:51:17Z

Test build #126357 has finished for PR 29159 at commit 52dd9b4.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-22T21:22:48Z

retest this please

viirya · 2020-07-22T22:26:14Z

python/pyspark/ml/tuning.py

@@ -545,6 +551,10 @@ class _TrainValidationSplitParams(_ValidatorParams):
    trainRatio = Param(Params._dummy(), "trainRatio", "Param for ratio between train and\
     validation data. Must be between 0 and 1.", typeConverter=TypeConverters.toFloat)

+    def __init__(self, *args):


You added *args many places. Just to make sure it is for consistency?

Yes, it's for consistency.

SparkQA · 2020-07-22T22:26:49Z

Test build #126362 has finished for PR 29159 at commit 52dd9b4.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2020-07-22T22:27:14Z

retest this please

viirya

Looks good overall.

SparkQA · 2020-07-22T23:18:26Z

Test build #126365 has finished for PR 29159 at commit 52dd9b4.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2020-07-22T23:21:01Z

retest this please

SparkQA · 2020-07-23T00:18:38Z

Test build #126366 has finished for PR 29159 at commit 52dd9b4.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-07-23T00:43:07Z

Jenkins, please make pip packaging tests pass! retest this please

SparkQA · 2020-07-23T01:50:37Z

Test build #126367 has finished for PR 29159 at commit 52dd9b4.

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

huaxingao · 2020-07-24T14:55:16Z

@srowen Are you Okay with this?

srowen · 2020-07-24T15:51:45Z

Same as the version for master - looks OK if there are no API or behavior changes (that are not bug fixes).

### What changes were proposed in this pull request? backporting the changes to 3.0 set params default values in trait Params for feature and tuning in both Scala and Python. ### Why are the changes needed? Make ML has the same default param values between estimator and its corresponding transformer, and also between Scala and Python. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing and modified tests Closes #29159 from huaxingao/set_default_3.0. Authored-by: Huaxin Gao <huaxing@us.ibm.com> Signed-off-by: Huaxin Gao <huaxing@us.ibm.com>

huaxingao · 2020-07-24T17:04:52Z

Merged to 3.0. Thank you all for reviewing!

huaxingao added 2 commits July 19, 2020 22:05

[SPARK-32310][ML][PySpark] ML params default value parity

31c88df

fix

f657d77

probot-autolabeler bot added ML PYTHON labels Jul 20, 2020

fix NaN compare

16abf9c

huaxingao changed the title ~~[SPARK-32310][ML][PySpark] ML params default value parity~~ [SPARK-32310][ML][PySpark][3.0] ML params default value parity Jul 20, 2020

zhengruifeng reviewed Jul 22, 2020

View reviewed changes

address comments

52dd9b4

viirya reviewed Jul 22, 2020

View reviewed changes

zhengruifeng approved these changes Jul 23, 2020

View reviewed changes

viirya approved these changes Jul 23, 2020

View reviewed changes

huaxingao closed this Jul 24, 2020

huaxingao deleted the set_default_3.0 branch July 24, 2020 17:05

[SPARK-32310][ML][PySpark][3.0] ML params default value parity #29159

[SPARK-32310][ML][PySpark][3.0] ML params default value parity #29159

Conversation

huaxingao commented Jul 20, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

huaxingao commented Jul 20, 2020

SparkQA commented Jul 20, 2020

SparkQA commented Jul 20, 2020

zhengruifeng commented Jul 20, 2020

SparkQA commented Jul 20, 2020

srowen commented Jul 20, 2020

SparkQA commented Jul 20, 2020

huaxingao commented Jul 20, 2020

SparkQA commented Jul 20, 2020

huaxingao commented Jul 20, 2020

SparkQA commented Jul 20, 2020

huaxingao commented Jul 20, 2020

SparkQA commented Jul 20, 2020

viirya commented Jul 20, 2020

viirya commented Jul 20, 2020

viirya commented Jul 20, 2020

huaxingao commented Jul 20, 2020 • edited Loading

SparkQA commented Jul 20, 2020

Fokko commented Jul 21, 2020 • edited Loading

huaxingao commented Jul 21, 2020

zhengruifeng Jul 22, 2020

Choose a reason for hiding this comment

huaxingao Jul 22, 2020

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2020

huaxingao commented Jul 22, 2020

SparkQA commented Jul 22, 2020

huaxingao commented Jul 22, 2020

SparkQA commented Jul 22, 2020

huaxingao commented Jul 22, 2020

SparkQA commented Jul 22, 2020

huaxingao commented Jul 22, 2020

SparkQA commented Jul 22, 2020

huaxingao commented Jul 22, 2020

SparkQA commented Jul 22, 2020

huaxingao commented Jul 22, 2020

viirya Jul 22, 2020

Choose a reason for hiding this comment

huaxingao Jul 22, 2020

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2020

viirya commented Jul 22, 2020

viirya left a comment

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2020

viirya commented Jul 22, 2020

SparkQA commented Jul 23, 2020

huaxingao commented Jul 23, 2020

SparkQA commented Jul 23, 2020

huaxingao commented Jul 24, 2020

srowen commented Jul 24, 2020

huaxingao commented Jul 24, 2020

huaxingao commented Jul 20, 2020 •

edited

Loading

Fokko commented Jul 21, 2020 •

edited

Loading