[SPARK-18619][ML] Make QuantileDiscretizer/Bucketizer/StringIndexer/RFormula inherit from HasHandleInvalid #18582

zhengruifeng · 2017-07-10T07:14:16Z

What changes were proposed in this pull request?

1, HasHandleInvaild support override
2, Make QuantileDiscretizer/Bucketizer/StringIndexer/RFormula inherit from HasHandleInvalid

How was this patch tested?

existing tests

JIRA

SparkQA · 2017-07-10T07:18:51Z

Test build #79447 has finished for PR 18582 at commit d336f14.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-07-10T08:19:13Z

Test build #79451 has finished for PR 18582 at commit 816c486.

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

SparkQA · 2017-07-11T03:39:17Z

Test build #79495 has finished for PR 18582 at commit 71c4250.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class Bucketizer(JavaTransformer, HasInputCol, HasOutputCol, HasHandleInvalid,
class QuantileDiscretizer(JavaEstimator, HasInputCol, HasOutputCol, HasHandleInvalid,
class RFormula(JavaEstimator, HasFeaturesCol, HasLabelCol, HasHandleInvalid,

SparkQA · 2017-07-11T04:56:37Z

Test build #79497 has finished for PR 18582 at commit bd467b6.

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

facaiy · 2017-07-11T09:45:11Z

mllib/src/main/scala/org/apache/spark/ml/feature/Bucketizer.scala

@@ -36,7 +36,8 @@ import org.apache.spark.sql.types.{DoubleType, StructField, StructType}
 */
 @Since("1.4.0")
 final class Bucketizer @Since("1.4.0") (@Since("1.4.0") override val uid: String)
-  extends Model[Bucketizer] with HasInputCol with HasOutputCol with DefaultParamsWritable {
+  extends Model[Bucketizer] with HasHandleInvalid with HasInputCol with HasOutputCol
+    with DefaultParamsWritable {


How about aligning with with extends?

I guess it was suggested by IDEA.

facaiy · 2017-07-11T09:49:51Z

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

@@ -460,16 +460,16 @@ object LinearRegression extends DefaultParamsReadable[LinearRegression] {
  val MAX_FEATURES_FOR_NORMAL_SOLVER: Int = WeightedLeastSquares.MAX_NUM_FEATURES

  /** String name for "auto". */
-  private[regression] val AUTO = "auto"
+  private[regression] val Auto = "auto"


Is solver related with handleInvalid? It seems a little confused.

@facaiy It's not related to this PR, just addressed other small issues by the way.

@facaiy We had some offline discussion, and decided to fix some small issues in this PR.

facaiy · 2017-07-11T09:59:08Z

python/pyspark/ml/feature.py

@@ -3058,26 +3035,37 @@ class RFormula(JavaEstimator, HasFeaturesCol, HasLabelCol, JavaMLReadable, JavaM
                                   "RFormula drops the same category as R when encoding strings.",
                                   typeConverter=TypeConverters.toString)

+    handleInvalid = Param(Params._dummy(), "handleInvalid", "how to handle invalid entries. " +


If I understand correctly, handleInvalid has been declared in HasHandleInvalid interface, right?

@facaiy We have to override it to keep the pydoc in line with scala side.

facaiy · 2017-07-11T10:02:22Z

mllib/src/main/scala/org/apache/spark/ml/feature/StringIndexer.scala

@@ -36,7 +36,8 @@ import org.apache.spark.util.collection.OpenHashMap
 /**
 * Base trait for [[StringIndexer]] and [[StringIndexerModel]].
 */
-private[feature] trait StringIndexerBase extends Params with HasInputCol with HasOutputCol {
+private[feature] trait StringIndexerBase extends Params with HasHandleInvalid with HasInputCol


Perhaps it is better to break the line at extends.

yanboliang · 2017-07-11T12:54:05Z

mllib/src/main/scala/org/apache/spark/ml/feature/Bucketizer.scala

@@ -36,7 +36,8 @@ import org.apache.spark.sql.types.{DoubleType, StructField, StructType}
 */
 @Since("1.4.0")
 final class Bucketizer @Since("1.4.0") (@Since("1.4.0") override val uid: String)
-  extends Model[Bucketizer] with HasInputCol with HasOutputCol with DefaultParamsWritable {
+  extends Model[Bucketizer] with HasHandleInvalid with HasInputCol with HasOutputCol
+    with DefaultParamsWritable {


I guess it was suggested by IDEA.

yanboliang · 2017-07-11T12:54:33Z

mllib/src/main/scala/org/apache/spark/ml/feature/Bucketizer.scala

@@ -86,15 +87,11 @@ final class Bucketizer @Since("1.4.0") (@Since("1.4.0") override val uid: String
   */
  // TODO: SPARK-18619 Make Bucketizer inherit from HasHandleInvalid.


Remove this line.

yanboliang · 2017-07-11T13:01:22Z

mllib/src/main/scala/org/apache/spark/ml/feature/QuantileDiscretizer.scala

@@ -74,16 +74,12 @@ private[feature] trait QuantileDiscretizerBase extends Params
   */
  // TODO: SPARK-18619 Make QuantileDiscretizer inherit from HasHandleInvalid.


Remove this line.

yanboliang · 2017-07-11T13:06:40Z

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

@@ -460,16 +460,16 @@ object LinearRegression extends DefaultParamsReadable[LinearRegression] {
  val MAX_FEATURES_FOR_NORMAL_SOLVER: Int = WeightedLeastSquares.MAX_NUM_FEATURES

  /** String name for "auto". */
-  private[regression] val AUTO = "auto"
+  private[regression] val Auto = "auto"


@facaiy It's not related to this PR, just addressed other small issues by the way.

yanboliang · 2017-07-11T13:37:06Z

python/pyspark/ml/feature.py

    @keyword_only
    def __init__(self, formula=None, featuresCol="features", labelCol="label",
-                 forceIndexLabel=False, stringIndexerOrderType="frequencyDesc"):
+                 forceIndexLabel=False, handleInvalid="error",
+                 stringIndexerOrderType="frequencyDesc"):


We should append new added argument in case to break users' code.

SparkQA · 2017-07-12T13:17:05Z

Test build #79555 has finished for PR 18582 at commit 61505f0.

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

yanboliang · 2017-07-12T14:08:20Z

LGTM, merged into master. Thanks.

zhengruifeng added 3 commits July 11, 2017 10:46

create pr

156cbd9

fix style

b6e7465

update py side

71c4250

zhengruifeng force-pushed the heritate_HasHandleInvalid branch from 816c486 to 71c4250 Compare July 11, 2017 03:05

fix py RFormula

bd467b6

facaiy reviewed Jul 11, 2017

View reviewed changes

yanboliang reviewed Jul 11, 2017

View reviewed changes

update py rformula

61505f0

asfgit closed this in d2d2a5d Jul 12, 2017

zhengruifeng deleted the heritate_HasHandleInvalid branch August 21, 2019 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-18619][ML] Make QuantileDiscretizer/Bucketizer/StringIndexer/RFormula inherit from HasHandleInvalid #18582

[SPARK-18619][ML] Make QuantileDiscretizer/Bucketizer/StringIndexer/RFormula inherit from HasHandleInvalid #18582

zhengruifeng commented Jul 10, 2017 •

edited

Loading

SparkQA commented Jul 10, 2017

SparkQA commented Jul 10, 2017

SparkQA commented Jul 11, 2017

SparkQA commented Jul 11, 2017

facaiy Jul 11, 2017

yanboliang Jul 11, 2017

facaiy Jul 11, 2017

yanboliang Jul 11, 2017

zhengruifeng Jul 12, 2017

facaiy Jul 11, 2017

zhengruifeng Jul 12, 2017

facaiy Jul 11, 2017

yanboliang Jul 11, 2017

yanboliang Jul 11, 2017

yanboliang Jul 11, 2017

yanboliang Jul 11, 2017

yanboliang Jul 11, 2017

SparkQA commented Jul 12, 2017

yanboliang commented Jul 12, 2017

		@@ -86,15 +87,11 @@ final class Bucketizer @Since("1.4.0") (@Since("1.4.0") override val uid: String
		*/
		// TODO: SPARK-18619 Make Bucketizer inherit from HasHandleInvalid.

		@@ -74,16 +74,12 @@ private[feature] trait QuantileDiscretizerBase extends Params
		*/
		// TODO: SPARK-18619 Make QuantileDiscretizer inherit from HasHandleInvalid.

[SPARK-18619][ML] Make QuantileDiscretizer/Bucketizer/StringIndexer/RFormula inherit from HasHandleInvalid #18582

[SPARK-18619][ML] Make QuantileDiscretizer/Bucketizer/StringIndexer/RFormula inherit from HasHandleInvalid #18582

Conversation

zhengruifeng commented Jul 10, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Jul 10, 2017

SparkQA commented Jul 10, 2017

SparkQA commented Jul 11, 2017

SparkQA commented Jul 11, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 12, 2017

yanboliang commented Jul 12, 2017

zhengruifeng commented Jul 10, 2017 •

edited

Loading